Summary: | [GTK] QoS support in the video sink | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Philippe Normand <pnormand> | ||||||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, slomo | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | Linux | ||||||||||||
Attachments: |
|
Description
Philippe Normand
2009-10-01 06:08:19 PDT
Too late is (IIRC) element's base time + buffer time + buffer duration >= element's clock time Created attachment 40621 [details]
proposed patch
Patch looks good to me but as said on IRC it's only a start, on top of this QoS as I've explained on IRC. This for now, will make playback go smoother and make sure that audio/video are displayed in sync. Created attachment 40640 [details]
updated patch
Comment on attachment 40640 [details] updated patch Awesome stuff, and I'm happy to see the real fix for the race condition happening! What do you think of moving this sink to WebKit/gtk/webkit/ like we did with soup-dialog, by the way? The synchronization and object life time handling look OK to me, I have just a couple nitpicks: > + // use HIGH_IDLE+20 priority, like Gtk+ for redrawing operations > + priv->timeout_id = g_timeout_add_full(G_PRIORITY_HIGH_IDLE + 20, 0, > + webkit_video_sink_timeout_func, > + gst_object_ref(sink), > + (GDestroyNotify) gst_object_unref); Additional space on the cast, and I suggest a normal sentence in the comment, with capital U and period =). > > static void > -webkit_video_sink_finalize(GObject* object) > +_unlock_buffer_mutex(WebKitVideoSinkPrivate* priv) > +{ > + > + g_mutex_lock(priv->buffer_mutex); > + There seems to be an unnecessary line at the start of this function. Also, I don't think we use this _name pattern anywhere else, so I'd suggest just dropping the underscore here. You are already declaring it static, which should be enough. Thanks for your work on this! Created attachment 40939 [details]
updated patch
In the timeout callback, no need to get the timeout id. But set the timeout id to 0 in the callback after taking the mutex! When unblocking the buffer waiting, you also want to remove the timeout if the id is != 0. Oh, and you want to make the video sink a subclass of GstVideoSink. This will actually set up the QoS/lateness logic in GstBaseSink, suitable for video: http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-base-libs/html/gst-plugins-base-libs-gstvideosink.html Created attachment 40940 [details]
updated patch
Comment on attachment 40940 [details]
updated patch
Thanks. And thanks to slomo for the meat of the review =).
Comment on attachment 40940 [details] updated patch Clearing flags on attachment: 40940 Committed r49401: <http://trac.webkit.org/changeset/49401> All reviewed patches have been landed. Closing bug. |