Bug 103898 - [GStreamer] create playbin in ::load(), not in player constructor
: [GStreamer] create playbin in ::load(), not in player constructor
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit Gtk
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks: 31155
  Show dependency treegraph
 
Reported: 2012-12-03 08:27 PST by Víctor M. Jáquez L.
Modified: 2012-12-16 07:42 PST (History)
10 users (show)

See Also:


Attachments
Patch (4.48 KB, patch)
2012-12-15 04:45 PST, Víctor M. Jáquez L.
no flags Details | Formatted Diff | Diff
Patch (4.48 KB, patch)
2012-12-15 05:10 PST, Víctor M. Jáquez L.
no flags Details | Formatted Diff | Diff
Patch (4.51 KB, patch)
2012-12-15 11:20 PST, Víctor M. Jáquez L.
no flags Details | Formatted Diff | Diff
Patch (4.48 KB, patch)
2012-12-16 05:06 PST, Víctor M. Jáquez L.
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Víctor M. Jáquez L. 2012-12-03 08:27:28 PST
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.
Comment 1 Víctor M. Jáquez L. 2012-12-15 04:45:33 PST
Created attachment 179587 [details]
Patch
Comment 2 Early Warning System Bot 2012-12-15 04:52:02 PST
Comment on attachment 179587 [details]
Patch

Attachment 179587 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15366013
Comment 3 Early Warning System Bot 2012-12-15 04:53:09 PST
Comment on attachment 179587 [details]
Patch

Attachment 179587 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15362036
Comment 4 EFL EWS Bot 2012-12-15 04:53:45 PST
Comment on attachment 179587 [details]
Patch

Attachment 179587 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15317988
Comment 5 Philippe Normand 2012-12-15 04:57:24 PST
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
Comment 6 Víctor M. Jáquez L. 2012-12-15 05:10:31 PST
Created attachment 179589 [details]
Patch
Comment 7 Víctor M. Jáquez L. 2012-12-15 06:36:17 PST
(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.
Comment 8 Philippe Normand 2012-12-15 07:07:26 PST
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.
Comment 9 Víctor M. Jáquez L. 2012-12-15 11:20:49 PST
Created attachment 179601 [details]
Patch
Comment 10 Philippe Normand 2012-12-15 11:30:52 PST
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" ?
Comment 11 Víctor M. Jáquez L. 2012-12-16 05:06:20 PST
Created attachment 179645 [details]
Patch
Comment 12 WebKit Review Bot 2012-12-16 07:42:51 PST
Comment on attachment 179645 [details]
Patch

Clearing flags on attachment: 179645

Committed r137838: <http://trac.webkit.org/changeset/137838>
Comment 13 WebKit Review Bot 2012-12-16 07:42:56 PST
All reviewed patches have been landed.  Closing bug.