Bug 103898

Summary: [GStreamer] create playbin in ::load(), not in player constructor
Product: WebKit Reporter: Víctor M. Jáquez L. <vjaquez>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: eric.carlson, feature-media-reviews, gustavo, menard, mrobinson, philn, pnormand, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 31155    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Víctor M. Jáquez L.
Reported 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.
Attachments
Patch (4.48 KB, patch)
2012-12-15 04:45 PST, Víctor M. Jáquez L. 		no flags
DetailsFormatted DiffDiff
Patch (4.48 KB, patch)
2012-12-15 05:10 PST, Víctor M. Jáquez L. 		no flags
DetailsFormatted DiffDiff
Patch (4.51 KB, patch)
2012-12-15 11:20 PST, Víctor M. Jáquez L. 		no flags
DetailsFormatted DiffDiff
Patch (4.48 KB, patch)
2012-12-16 05:06 PST, Víctor M. Jáquez L. 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Víctor M. Jáquez L.
Comment 1 2012-12-15 04:45:33 PST
Created attachment 179587 [details] Patch
Early Warning System Bot
Comment 2 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
Early Warning System Bot
Comment 3 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
EFL EWS Bot
Comment 4 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
Philippe Normand
Comment 5 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
Víctor M. Jáquez L.
Comment 6 2012-12-15 05:10:31 PST
Created attachment 179589 [details] Patch
Víctor M. Jáquez L.
Comment 7 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.
Philippe Normand
Comment 8 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.
Víctor M. Jáquez L.
Comment 9 2012-12-15 11:20:49 PST
Created attachment 179601 [details] Patch
Philippe Normand
Comment 10 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" ?
Víctor M. Jáquez L.
Comment 11 2012-12-16 05:06:20 PST
Created attachment 179645 [details] Patch
WebKit Review Bot
Comment 12 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>
WebKit Review Bot
Comment 13 2012-12-16 07:42:56 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.