Bug 103898 – [GStreamer] create playbin in ::load(), not in player constructor

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

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

Following the bug #31155 we need to create playbin in ::load(), not in player constructor. Hence we could build the pipeline after we have the most of its run-time requirements.

I set this bug as subtask of the bug #31155, as it might be a risky change.

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

Created attachment 179587 [details]
Patch

[Early Warning System Bot]

Comment on attachment 179587 [details]
Patch

Attachment 179587 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15366013

[Early Warning System Bot]

Comment on attachment 179587 [details]
Patch

Attachment 179587 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15362036

[EFL EWS Bot]

Comment on attachment 179587 [details]
Patch

Attachment 179587 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15317988

[Philippe Normand]

Comment on attachment 179587 [details]
Patch

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

Looks good I guess, I'd like to test it before landing. Are the tests still passing? including http/tests/media ?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1774
> +    enableDownloadBuffering();

s/enable/set I guess

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

Created attachment 179589 [details]
Patch

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

(In reply to comment #5)
> (From update of attachment 179587 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179587&action=review
> 
> Looks good I guess, I'd like to test it before landing. Are the tests still passing? including http/tests/media ?
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1774
> > +    enableDownloadBuffering();
> 
> s/enable/set I guess

Yep

All the tests passed ok

$ Tools/Scripts/run-webkit-tests --gtk http/test/media
Using port 'gtk'
Test configuration: <, x86, release>
Placing test results in /home/vjaquez/WebKit/WebKitBuild/Release/layout-test-results
Baseline search path: gtk -> generic
Using Release build
Pixel tests disabled
Regular timeout: 6000, slow test timeout: 30000
Command line: /home/vjaquez/WebKit/WebKitBuild/Release/Programs/DumpRenderTree -

Found 39 tests; running 21, skipping 18.   
Running 1 DumpRenderTree over 1 shard.

All 21 tests ran as expected.

[Philippe Normand]

Comment on attachment 179589 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:-1753
> -    ASSERT(m_playBin);

I think it'd make sense to move this ASSERT in ::load, before setting the url of playbin.

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

Created attachment 179601 [details]
Patch

[Philippe Normand]

Comment on attachment 179601 [details]
Patch

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

Some more feedback, I'd still like to test this patch as well :)

> Source/WebCore/ChangeLog:8
> +        This patch does the instantiation of the playbin pipeline into the  WebCore::MediaPlayerPrivateGStreamer::load() method, so we could construct the pipeline take in count other possible options, such as pitch.

Sorry for the late feedback on this, can you please wrap this line so it's not ultra wide huge? :)

Also rephrase the second part, such as "so the pipeline layout can reflect other predefined settings such as audio pitch preservation in variable playback rate scenarios" ?

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

Created attachment 179645 [details]
Patch

[WebKit Review Bot]

Comment on attachment 179645 [details]
Patch

Clearing flags on attachment: 179645

Committed r137838: <http://trac.webkit.org/changeset/137838>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.