Bug 220651

Summary:

[GStreamer] Clean-up the TextSink

Product:

WebKit

Reporter:

Philippe Normand <pnormand>

Component:

Platform

Assignee:

Philippe Normand <pnormand>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

calvaris, cgarcia, eric.carlson, ews-watchlist, glenn, gustavo, jbedard, jer.noble, menard, philipj, sergio, vjaquez, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	calvaris: review+, calvaris: commit-queue-

Description Philippe Normand 2021-01-15 01:49:26 PST

Currently the text-sink implementation is spread between a custom AppSink sub-class and some AppSink code in the player. In order to reduce clutter in the player, this sink could be a bin and the implementation could host the appsink implementation details.

Comment 1 Philippe Normand 2021-01-15 02:41:11 PST

Created attachment 417691 [details]
Patch

Comment 2 Xabier Rodríguez Calvar 2021-01-15 02:50:43 PST

Comment on attachment 417691 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417691&action=review

> Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp:84
> +    g_object_set(priv->appSink.get(), "emit-signals", true, "enable-last-sample", false, "caps", textCaps.get(), nullptr);

I think these should be TRUE and FALSE

> Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp:88
> +        auto sample = adoptGRef(gst_app_sink_pull_sample(GST_APP_SINK(appSink)));
> +        webkitTextSinkHandleSample(sink, WTFMove(sample));

What about webkitTextSinkHandleSample(sink, adoptGRef(gst_app_sink_pull_sample(GST_APP_SINK(appSink)))); ?

> Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp:94
> +        auto sample = adoptGRef(gst_app_sink_pull_preroll(GST_APP_SINK(appSink)));
> +        webkitTextSinkHandleSample(sink, WTFMove(sample));

Ditto.

Comment 3 Philippe Normand 2021-01-15 03:11:30 PST

WeakPtr asserts in debug, this needs another iteration :)

Comment 4 Philippe Normand 2021-01-15 03:49:20 PST

Created attachment 417692 [details]
Patch

Comment 5 Xabier Rodríguez Calvar 2021-01-15 05:25:24 PST

Comment on attachment 417692 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417692&action=review

> Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp:130
> +GstElement* webkitTextSinkNew(WeakPtr<MediaPlayerPrivateGStreamer> player)

I think you can && here and move when assigning to the priv

Comment 6 Philippe Normand 2021-01-15 06:00:00 PST

Committed r271517: <https://trac.webkit.org/changeset/271517>

Comment 7 Radar WebKit Bug Importer 2021-01-15 06:00:42 PST

<rdar://problem/73246106>