Bug 220651

Summary: [GStreamer] Clean-up the TextSink
Product: WebKit Reporter: Philippe Normand <pnormand>
Component: PlatformAssignee: 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: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch calvaris: review+, calvaris: commit-queue-

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.