Bug 213029 – [GStreamer] Avoid setting GstContext twice in GLVideoSinkGStreamer

• Component: Media
• Status: RESOLVED FIXED

[Víctor M. Jáquez L.]

When the GL video sink changes it state, it might set in the bin its context twice.

[Víctor M. Jáquez L.]

Created attachment 401549 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 401549 [details]
Patch

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

I guess a GstContext should disappear at some point when we transition down to a certain state, I don't know if this is READY or NULL but this element is not handling that. If context disappears in READY in the sub elements, you won't be setting it again, or am I missing anything? I guess we should check this, not in the scope of this patch. Besides, at some point I guess we should clear the context at some state here, right?

This patch seems to be ok considering only this piece of code but please consider the rest I'm saying.

> Source/WebCore/ChangeLog:3
> +        [GStreamer] Avoid set gstcontext twice in GLVideoSinkGStreamer

GstContext, please change the bug title in bugzilla.

> Source/WebCore/ChangeLog:10
> +        No new tests as there is no change in functionality.

I guess setting the GstContext twice does not create problems, right? If it creates problems it would be a bug and that would require a test.

[Víctor M. Jáquez L.]

Created attachment 402097 [details]
Patch

[Víctor M. Jáquez L.]

I've refactored the patch, so it might need a second look

[Víctor M. Jáquez L.]

(In reply to Xabier Rodríguez Calvar from comment #2)
> Comment on attachment 401549 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401549&action=review
> 
> I guess a GstContext should disappear at some point when we transition down
> to a certain state, I don't know if this is READY or NULL but this element
> is not handling that. If context disappears in READY in the sub elements,
> you won't be setting it again, or am I missing anything? I guess we should
> check this, not in the scope of this patch. Besides, at some point I guess
> we should clear the context at some state here, right?

if contexts are persistent (and ours are!) they are removed until dispose: https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/master/gst/gstelement.c#L3376

(non persistent contexts are removed when state changes from READY to NULL: https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/master/gst/gstelement.c#L3266 )

The problem setting them several times, at least, in a waste of cpu cycles because when they are set, the bin set them again to  its children, and so on.

[Xabier Rodríguez Calvar]

Comment on attachment 402097 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:161
> +        auto setContext = [&] (const gchar *contextType) {

Nit: make this a regular function and you're done.

[Víctor M. Jáquez L.]

Created attachment 402113 [details]
Patch

[Philippe Normand]

If you want to land this with cq I'm afraid you'll need to re-upload the patch...

[EWS]

Committed r263217: <https://trac.webkit.org/changeset/263217>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402113 [details].

[Radar WebKit Bug Importer]

<rdar://problem/64494905>

[Aakash Jain]

(In reply to Philippe Normand from comment #8)
> If you want to land this with cq I'm afraid you'll need to re-upload the patch...
I believe you fixed the issue by removing and setting the cq+ flag. I have improved the commit-queue in Bug 213377 so that it can better handle such trac downtime.

Also, my aim is that no-one should have to re-upload a patch just because of EWS. Please file a bug if you come across any scenario requiring re-upload of patch because of any EWS issue.