RESOLVED FIXED Bug 220651
[GStreamer] Clean-up the TextSink 
https://bugs.webkit.org/show_bug.cgi?id=220651
Summary [GStreamer] Clean-up the TextSink
Philippe Normand
Reported 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.
Attachments
Patch (16.92 KB, patch)
2021-01-15 02:41 PST, Philippe Normand 		no flags
DetailsFormatted DiffDiff
Patch (17.09 KB, patch)
2021-01-15 03:49 PST, Philippe Normand 		calvaris: review+
calvaris: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Philippe Normand
Comment 1 2021-01-15 02:41:11 PST
Created attachment 417691 [details] Patch
Xabier Rodríguez Calvar
Comment 2 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.
Philippe Normand
Comment 3 2021-01-15 03:11:30 PST
WeakPtr asserts in debug, this needs another iteration :)
Philippe Normand
Comment 4 2021-01-15 03:49:20 PST
Created attachment 417692 [details] Patch
Xabier Rodríguez Calvar
Comment 5 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
Philippe Normand
Comment 6 2021-01-15 06:00:00 PST
Committed r271517: <https://trac.webkit.org/changeset/271517>
Radar WebKit Bug Importer
Comment 7 2021-01-15 06:00:42 PST
<rdar://problem/73246106>
Note You need to log in before you can comment on or make changes to this bug.