Bug 103898 - [GStreamer] create playbin in ::load(), not in player constructor
: [GStreamer] create playbin in ::load(), not in player constructor
Status: RESOLVED FIXED
: WebKit
WebKit Gtk
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
: 31155
  Show dependency treegraph
 
Reported: 2012-12-03 08:27 PST by
Modified: 2012-12-16 07:42 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2012-12-15 04:45:33 PST -------
Created an attachment (id=179587) [details]
Patch
------- Comment #2 From 2012-12-15 04:52:02 PST -------
(From update of attachment 179587 [details])
Attachment 179587 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15366013
------- Comment #3 From 2012-12-15 04:53:09 PST -------
(From update of attachment 179587 [details])
Attachment 179587 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15362036
------- Comment #4 From 2012-12-15 04:53:45 PST -------
(From update of attachment 179587 [details])
Attachment 179587 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15317988
------- Comment #5 From 2012-12-15 04:57:24 PST -------
(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
------- Comment #6 From 2012-12-15 05:10:31 PST -------
Created an attachment (id=179589) [details]
Patch
------- Comment #7 From 2012-12-15 06:36:17 PST -------
(In reply to comment #5)
> (From update of attachment 179587 [details] [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 From 2012-12-15 07:07:26 PST -------
(From update of attachment 179589 [details])
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 From 2012-12-15 11:20:49 PST -------
Created an attachment (id=179601) [details]
Patch
------- Comment #10 From 2012-12-15 11:30:52 PST -------
(From update of attachment 179601 [details])
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 From 2012-12-16 05:06:20 PST -------
Created an attachment (id=179645) [details]
Patch
------- Comment #12 From 2012-12-16 07:42:51 PST -------
(From update of attachment 179645 [details])
Clearing flags on attachment: 179645

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