Bug 116042

Summary: [GStreamer] [texmap] upload a video buffer into the video texture
Product: WebKit Reporter: Víctor M. Jáquez L. <vjaquez>
Component: MediaAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: b.long, commit-queue, eflews.bot, ehdgms, eric.carlson, glenn, gns, gtk-ews, gyuyoung.kim, hk, jer.noble, menard, mrobinson, pnormand, rego+ews, slomo, webkit-ews, xan.lopez, zan, zarvai
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 86410    
Bug Blocks:    
Attachments:
Description Flags
upload the buffer with GstSurface{Meta/Convert} (GStreamer 1.0 -deprecated-)
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Unreviewed, build fix after r151673
none
Fix after r151673
none
gstreamer log none

Description Víctor M. Jáquez L. 2013-05-13 09:52:05 PDT
In bug #86410, composited video support, using the TexturMapper, was added. But it doesn't handle when the video is handled by Accelerated Hardware decoders.

Is until GStreamer 1.2 when we have official support to upload a buffer into a texture without changing from GPU context to the CPU one, hence webkit would play fullHD videos.
Comment 1 Víctor M. Jáquez L. 2013-05-13 09:55:51 PDT
Created attachment 201579 [details]
upload the buffer with GstSurface{Meta/Convert} (GStreamer 1.0 -deprecated-)

This patch enables the purpose of this bug, but uses deprecated GStreamer API.
Comment 2 WebKit Commit Bot 2013-05-13 09:58:47 PDT
Attachment 201579 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h', u'Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp', u'Source/autotools/FindDependencies.m4']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:43:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 EFL EWS Bot 2013-05-13 09:59:57 PDT
Comment on attachment 201579 [details]
upload the buffer with GstSurface{Meta/Convert} (GStreamer 1.0 -deprecated-)

Attachment 201579 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/404791
Comment 4 Early Warning System Bot 2013-05-13 10:00:08 PDT
Comment on attachment 201579 [details]
upload the buffer with GstSurface{Meta/Convert} (GStreamer 1.0 -deprecated-)

Attachment 201579 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/375833
Comment 5 EFL EWS Bot 2013-05-13 10:01:26 PDT
Comment on attachment 201579 [details]
upload the buffer with GstSurface{Meta/Convert} (GStreamer 1.0 -deprecated-)

Attachment 201579 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/461779
Comment 6 Early Warning System Bot 2013-05-13 10:01:47 PDT
Comment on attachment 201579 [details]
upload the buffer with GstSurface{Meta/Convert} (GStreamer 1.0 -deprecated-)

Attachment 201579 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/451832
Comment 7 Víctor M. Jáquez L. 2013-06-07 09:04:27 PDT
Created attachment 204047 [details]
Patch
Comment 8 Víctor M. Jáquez L. 2013-06-07 09:07:13 PDT
I've two questions regarding this patch:

1) Shall I maintain the deprecated/unofficial GstSurfaceMeta/GstSurfaceConvert API?

If we remove it, the code will be simpler and more easy to read.

2) Should I add the support got GstCapsFeatures?

I guess we could do this after this patch get landed.
Comment 9 Early Warning System Bot 2013-06-07 09:09:21 PDT
Comment on attachment 204047 [details]
Patch

Attachment 204047 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/777474
Comment 10 EFL EWS Bot 2013-06-07 09:09:28 PDT
Comment on attachment 204047 [details]
Patch

Attachment 204047 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/793136
Comment 11 Early Warning System Bot 2013-06-07 09:09:49 PDT
Comment on attachment 204047 [details]
Patch

Attachment 204047 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/777473
Comment 12 EFL EWS Bot 2013-06-07 09:10:11 PDT
Comment on attachment 204047 [details]
Patch

Attachment 204047 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/774532
Comment 13 Víctor M. Jáquez L. 2013-06-07 09:22:38 PDT
Yeah, if we go supporting the deprecated GstSurfaceMeta, I also need to modify the cmake scripts...
Comment 14 kov's GTK+ EWS bot 2013-06-07 09:25:53 PDT
Comment on attachment 204047 [details]
Patch

Attachment 204047 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/793139
Comment 15 Philippe Normand 2013-06-07 09:41:30 PDT
(In reply to comment #8)
> I've two questions regarding this patch:
> 
> 1) Shall I maintain the deprecated/unofficial GstSurfaceMeta/GstSurfaceConvert API?
> 
> If we remove it, the code will be simpler and more easy to read.
> 

Yes let's remove it. We don't want to maintain deprecated API usage :)

> 2) Should I add the support got GstCapsFeatures?
> 
> I guess we could do this after this patch get landed.

Yes sounds like a good idea!
Comment 16 Víctor M. Jáquez L. 2013-06-07 12:39:07 PDT
Created attachment 204060 [details]
Patch
Comment 17 EFL EWS Bot 2013-06-07 12:42:47 PDT
Comment on attachment 204060 [details]
Patch

Attachment 204060 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/709943
Comment 18 Early Warning System Bot 2013-06-07 12:44:42 PDT
Comment on attachment 204060 [details]
Patch

Attachment 204060 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/784156
Comment 19 Early Warning System Bot 2013-06-07 12:46:00 PDT
Comment on attachment 204060 [details]
Patch

Attachment 204060 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/709944
Comment 20 EFL EWS Bot 2013-06-07 12:47:08 PDT
Comment on attachment 204060 [details]
Patch

Attachment 204060 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/779180
Comment 21 Víctor M. Jáquez L. 2013-06-07 13:04:56 PDT
Created attachment 204061 [details]
Patch
Comment 22 Martin Robinson 2013-06-07 14:24:51 PDT
Comment on attachment 204061 [details]
Patch

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

Is the purpose of this patch to more quickly prepare the texture in the case that the video is already in GPU memory?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:356
> +    if (meta) {
> +        const BitmapTextureGL* textureGL = static_cast<const BitmapTextureGL*>(m_texture.get());
> +        guint ids[4] = { textureGL->id(), 0, 0, 0 };
> +
> +        ret = gst_video_gl_texture_upload_meta_upload(meta, ids);

What's happening behind the scenes here? Is it just using the current OpenGL context? If not I wonder about the cost of context switching.

In WebKit we do early returns, so you should write:

if (GstVideoGLTextureUploadMeta* meta = gst_buffer_get_video_gl_texture_upload_meta(buffer)) {
    if (gst_video_gl_texture_upload_meta_upload(meta, ids)) {
        client()->setPlatformLayerNeedsDisplay();
        return;
    }
}

The nice benefit being you don't have to touch the code below it.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:362
> +    { // copy the buffer into the texture

Please use full sentences with periods and capital letters in comments.
Comment 23 Martin Robinson 2013-06-07 14:24:53 PDT
Comment on attachment 204061 [details]
Patch

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

Is the purpose of this patch to more quickly prepare the texture in the case that the video is already in GPU memory?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:356
> +    if (meta) {
> +        const BitmapTextureGL* textureGL = static_cast<const BitmapTextureGL*>(m_texture.get());
> +        guint ids[4] = { textureGL->id(), 0, 0, 0 };
> +
> +        ret = gst_video_gl_texture_upload_meta_upload(meta, ids);

What's happening behind the scenes here? Is it just using the current OpenGL context? If not I wonder about the cost of context switching.

In WebKit we do early returns, so you should write:

if (GstVideoGLTextureUploadMeta* meta = gst_buffer_get_video_gl_texture_upload_meta(buffer)) {
    if (gst_video_gl_texture_upload_meta_upload(meta, ids)) {
        client()->setPlatformLayerNeedsDisplay();
        return;
    }
}

The nice benefit being you don't have to touch the code below it.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:362
> +    { // copy the buffer into the texture

Please use full sentences with periods and capital letters in comments.
Comment 24 Víctor M. Jáquez L. 2013-06-10 04:21:41 PDT
Created attachment 204151 [details]
Patch
Comment 25 Víctor M. Jáquez L. 2013-06-10 04:23:05 PDT
This last version of the patch aims Martin's review. Thanks!
Comment 26 Víctor M. Jáquez L. 2013-06-11 02:24:51 PDT
Comment on attachment 204061 [details]
Patch

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

>>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:356
>>> +        ret = gst_video_gl_texture_upload_meta_upload(meta, ids);
>> 
>> What's happening behind the scenes here? Is it just using the current OpenGL context? If not I wonder about the cost of context switching.
>> 
>> In WebKit we do early returns, so you should write:
>> 
>> if (GstVideoGLTextureUploadMeta* meta = gst_buffer_get_video_gl_texture_upload_meta(buffer)) {
>>     if (gst_video_gl_texture_upload_meta_upload(meta, ids)) {
>>         client()->setPlatformLayerNeedsDisplay();
>>         return;
>>     }
>> }
>> 
>> The nice benefit being you don't have to touch the code below it.
> 
> What's happening behind the scenes here? Is it just using the current OpenGL context? If not I wonder about the cost of context switching.
> 
> In WebKit we do early returns, so you should write:
> 
> if (GstVideoGLTextureUploadMeta* meta = gst_buffer_get_video_gl_texture_upload_meta(buffer)) {
>     if (gst_video_gl_texture_upload_meta_upload(meta, ids)) {
>         client()->setPlatformLayerNeedsDisplay();
>         return;
>     }
> }
> 
> The nice benefit being you don't have to touch the code below it.

Martin, Yes, it is using the current OpenGL context. Behind the scenes were a calling a this function vaCopySurfaceGLX: http://www.sourcecodebrowser.com/libva/1.0.4/va__glx_8h.html#ab3e822b68bbb2da0aadc25911ad42aae
Comment 27 Víctor M. Jáquez L. 2013-06-12 03:43:27 PDT
Created attachment 204424 [details]
Patch
Comment 28 Martin Robinson 2013-06-12 10:05:56 PDT
Comment on attachment 204424 [details]
Patch

Looks good to me, but before landing would love to see a review by Philippe and/or slomo.
Comment 29 Sebastian Dröge (slomo) 2013-06-12 23:42:49 PDT
Comment on attachment 204424 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:353
> +            if (gst_video_gl_texture_upload_meta_upload(meta, ids)) {

This looks all good

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:44
> +#define GST_SURFACE_CAPS "video/x-surface, opengl=true; "

Erm... no. There should not be any video/x-surface caps in 1.0 anywhere. Maybe you mean "video/x-raw(meta:GstVideoGLTextureMapper)" here?
Comment 30 Sebastian Dröge (slomo) 2013-06-12 23:44:05 PDT
(In reply to comment #29)

> > Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:44
> > +#define GST_SURFACE_CAPS "video/x-surface, opengl=true; "
> 
> Erm... no. There should not be any video/x-surface caps in 1.0 anywhere. Maybe you mean "video/x-raw(meta:GstVideoGLTextureMapper)" here?

And for that you should use GST_VIDEO_CAPS_MAKE_WITH_FEATURES() and also specify that you want BGRA/BGRx.
Comment 31 Philippe Normand 2013-06-13 03:17:37 PDT
Thanks for the review Sebastian :) Usually though only "official reviewers" are allowed to r+/r-. Anyway, props for the valuable comments on the patch.
Comment 32 Víctor M. Jáquez L. 2013-06-13 04:14:13 PDT
Comment on attachment 204424 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:44
>> +#define GST_SURFACE_CAPS "video/x-surface, opengl=true; "
> 
> Erm... no. There should not be any video/x-surface caps in 1.0 anywhere. Maybe you mean "video/x-raw(meta:GstVideoGLTextureMapper)" here?

Currently only gstreamer-vaapi can offer the GstVideoGLTextureMapper meta, but it won't offer x-raw buffers, only the custom caps video/x-surface:
https://bugzilla.gnome.org/show_bug.cgi?id=687183#c22

I can add both, otherwise this patch will be useless for a while.
Comment 33 Sebastian Dröge (slomo) 2013-06-14 02:08:35 PDT
(In reply to comment #31)
> Thanks for the review Sebastian :) Usually though only "official reviewers" are allowed to r+/r-. Anyway, props for the valuable comments on the patch.

Ah, I assumed that it would be ok because I apparently had permissions for that :) Sorry
I hope that doesn't mean that random people can also cq+? ;)

(In reply to comment #32)
> (From update of attachment 204424 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=204424&action=review
> 
> >> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:44
> >> +#define GST_SURFACE_CAPS "video/x-surface, opengl=true; "
> > 
> > Erm... no. There should not be any video/x-surface caps in 1.0 anywhere. Maybe you mean "video/x-raw(meta:GstVideoGLTextureMapper)" here?
> 
> Currently only gstreamer-vaapi can offer the GstVideoGLTextureMapper meta, but it won't offer x-raw buffers, only the custom caps video/x-surface:
> https://bugzilla.gnome.org/show_bug.cgi?id=687183#c22
> 
> I can add both, otherwise this patch will be useless for a while.

Can you add video/x-raw to gstreamer-vaapi instead please? We really don't want caps such as video/x-surface anymore in 1.0 and I don't think it's a good idea to lead by bad example here
Comment 34 Philippe Normand 2013-06-14 03:28:01 PDT
(In reply to comment #33)
> (In reply to comment #31)
> > Thanks for the review Sebastian :) Usually though only "official reviewers" are allowed to r+/r-. Anyway, props for the valuable comments on the patch.
> 
> Ah, I assumed that it would be ok because I apparently had permissions for that :) Sorry
> I hope that doesn't mean that random people can also cq+? ;)
> 

People with EditFlags permissions can set whatever flag they want but if the patch is landed with the commit-queue the bot with check:

- cq+ was set by a commiter
- r+ was set by a reviewer

If a patch was r+'ed with nitpicks the patch author can reupload the patch with updated Reviewed by field and set the cq flag only :)
Comment 35 Víctor M. Jáquez L. 2013-06-17 10:23:43 PDT
Created attachment 204835 [details]
Patch
Comment 36 Martin Robinson 2013-06-17 10:50:18 PDT
Comment on attachment 204835 [details]
Patch

Looks good to me as long as it's okay for the GStreamer experts.
Comment 37 WebKit Commit Bot 2013-06-18 01:10:12 PDT
Comment on attachment 204835 [details]
Patch

Clearing flags on attachment: 204835

Committed r151673: <http://trac.webkit.org/changeset/151673>
Comment 38 WebKit Commit Bot 2013-06-18 01:10:19 PDT
All reviewed patches have been landed.  Closing bug.
Comment 39 Sebastian Dröge (slomo) 2013-06-18 02:04:00 PDT
Yes looks good to me, a bit late comment but better late then never :)
Comment 40 Zoltan Arvai 2013-06-18 04:13:02 PDT
It seems some compositing video tests failing after the patch.

Video missing or test timed out on Qt WK1:
http://build.webkit.org/results/Qt%20Linux%20Release/r151676%20%2860915%29/results.html
Qt WK2:
http://build.webkit.sed.hu/results/x86-32%20Linux%20Qt%20Release%20WebKit2/r151674%20%2836237%29/results.html

Maybe EFL also affected, but I'm not sure because there are more problems:
http://build.webkit.org/results/EFL%20Linux%2064-bit%20Release%20WK2/r151676%20%289072%29/results.html

On GTK bot media test timing out massivly after r151673:
http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug%20WK1/r151673%20%282631%29/results.html

Can you check it, please?
Comment 41 Philippe Normand 2013-06-18 04:29:20 PDT
Victor is preparing a patch for this, it will be uploaded in this bug. Sorry for the breakage.
Comment 42 Víctor M. Jáquez L. 2013-06-18 04:31:32 PDT
Created attachment 204901 [details]
Unreviewed, build fix after r151673
Comment 43 Víctor M. Jáquez L. 2013-06-18 04:35:41 PDT
Created attachment 204902 [details]
Fix after r151673
Comment 44 Philippe Normand 2013-06-18 04:56:21 PDT
Reopening
Comment 45 WebKit Commit Bot 2013-06-18 05:24:23 PDT
Comment on attachment 204902 [details]
Fix after r151673

Clearing flags on attachment: 204902

Committed r151681: <http://trac.webkit.org/changeset/151681>
Comment 46 WebKit Commit Bot 2013-06-18 05:24:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 47 Brendan Long 2013-06-25 15:53:03 PDT
This patch causes certain videos to stop rendering for me using GStreamer master on Ubuntu 12.04 / Intel i7 graphics. Here's an example video:

https://www.dropbox.com/s/x03ogh3epbd6lfq/example.mkv

The audio plays but the video doesn't (and playbin never emits "video-changed").
Comment 48 Brendan Long 2013-06-25 16:09:12 PDT
I get this warning:

0:00:00.142290527 29837      0x10da800 WARN       webkitmediaplayer /home/blong/workspace/webkit/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:726:handleMessage: Warning 6: No decoder available for type 'video/x-theora, width=(int)320, height=(int)240, framerate=(fraction)30/1, streamheader=(buffer)< 807468656f72610302010014000f0001400000f000000000001e0000000100000100000100000000c0c0, 817468656f72612b000000586970682e4f7267206c69627468656f726120312e312032303039303832322028546875736e656c64612900000000, 827468656f7261becd28f7b9cd6b18b5a9494a10739ce6318c5294a42108318c62108421084000000000000000000011f4e1642e5549b47612f570b4986b95a2a9409648a1d047d399b8d66430174b2552912c94462210078391c0d06431168b05628130944621100783a1c0c8602c1509848220f0683014fb9917d69555541412d2d19190504f0f0dcdcc8c8b4b4b4a0a0a08c8c8c787878786464646450505050503c3c3c3c3c3c3c28282828282828281414141414141402100b0a101828333d0c0c0e131a3a3c370e0d1018283945380e11161d3357503e1216253a446d674d182337405168715c31404e5767797865485c5f62706467631112182f6363636312151a4263636363181a3863636363632f42636363636363636363636363636363636363636363636363636363636363636363636363636310101014181c2028101014181c2028301014181c2028304014181c2028304040181c2028304040401c20283040404060202830404040608028304040406080803e2fcb7d00d3313be428de0ea89bdb96622dd471be03f7289225ee0d8c97d6aa91a665762ec4204e460f68ea64be730dad586c655295e7dbb87e49dc434ccaec5d88217c8a3c355a836325e3f3977312ba3da3c9384fbaaa420c57354fbaaa457634119ca6941ed1e75dc90e0656b31b7ce1fc5785e0979dbfe138ed30f68f3a9226301756b3329a835df0c9690a3fb40bd0a78d8e65d91a077283da3ceb94db6bbd82b829546045f127fc5a2d63907dc221dcdaff710b48d8662cad01ba72572287bb79d497aaa4a84799c274ad66c171bc508aec0d240e4f143ddbaefffb263796c1483d9e05a759dc7162b3187c065472346a26964daffea52dcec8e2c3e0f93826964da3c16f019f5740b2b022d31e4caffea52dc84d68f5e76036a22ff81ec7cbe84dcc2767d2db056b8134b26ba6280f32777aaa440126bf5ee5ea7134bb506e759b0896a1b8595869fe28f23bf3855494a02e8ceb26251afc2cac5e4e4d2ed23cedc398843b8f7360ff4b6a29404573862f5aca2cac7922fedbb909b6a3ecc4661d09ffd325b0d452811c22cacdd79f6a4f33f6d42225cd8742d327ba705cc7c48e7ba8aa916562f943ef7c06d44ffebd33c23114832e18a6e6d171de9bc3005f5acd440750f0dbe5d50a715adcf393b447a4aa26923fcd32309f30be7576043f9d3b8b6c573e8053534b438728c2695ed1279ae461be60d5cc37b49caec637011edfb83b88133da2593ceaa539993e816b0dc079d9b05137b2c913586f6f395e2b33072ea3d325c7fc7c554ad61007d7b1809f26e35ed174e611656b980f095b144dec91e85acdff0aa914be2379dfe4ee334160aee6ec0352636a3d96489a745ac9cc2637d42aa465c9d6709fe370708c26286dafb2e9dd8aed20dcbe7093c8b0f841529237785a1fb149e56b1c4e3975121b9b8660d286dafb34b95ff0a5c4e3166d11232426976d6e39bcefdf07d6acac144387fb8cba82a15521db404d2ed26b47370ce1be01f5a7e7a8e2eac2b3cc276e658f029112806d34bac9a42bfa383dcbc5a61df23eb1598b38ff204f737a82caea53b84f7118266dacba49942e0bfdf07428cf0ed6fc726317f00aa9391264c31f2037707b15a3af90bb9c46500db59b4973bd77eb2b155242b5ecfb90387d18f177eb2b3a2f93b7230c41a36d66ce5519442aa5b80cda36d72c65bbe8481c3c2f60a26fc4562b49d9d2d7b3a3e441529703d188db564d33a30bfbb0dd0ab5a9ee2345d0f25904dffdc83879aee9ef3139226d443033ac594aac8b9a1d3b09a593d8ff3ec2d0f887ce18de65b9f9da3d799c595ae347488134bb49ed1814d46705a2d5f01f38fdbce068a09095b4dafbe6170238bbe72acb2a52c59ba843e65852a8fdd4300b98f0deb2cb4bc095336d64d23cf93c7689ed1387d752a8e5906895336d64f637f783b44a33f0eb0e16d85703731867103e7fca9a85a8671d9ed0331a4cdb5964c9d5f5962ef0e0c70451ee3727fd77282a290321b6a99a672d0c70edd4559621f8f119ec97b8b46e8568bd40c9a666dae2c269fb40aec29a845f1d49c8fc97dba379c0aa91d5fc2730c791f5a10d1dfe80668836d52269748f2f4b2b2ef3714ee79c2df808b286e80e578482695b6ba671e0fddb1a0b2b277bd54297c31241b6a95a4d9e5442ddc3af43f308d15fe2bb04fcad5dda0a86f529803b5d1705bd3ef4425e48666daa569364c70ffc394ea235958dea5287808fa122f60c590db54cd3397ff601ca75d416fd5bc588bb1e52a9c1217a56036d4ca6699e3727b4428b9807af947eb2b2377d6e3ca948b7e91cfa4085656460bc99934cd81b6b8ddff510ed3a5a38dc4672174c5e60cb134cdb6b45bec8aa0291d7762b4a4fffc42e226fe0a47b09a51d404ec383fcbe1bdc62cf9acb02224256d36b1f7e5eb6a15525c65df81182695b6ba499a2a2ff83f75eb8618a0dcb6c5643cce2aa42324253499b6b9d17b963810f9f03b6517f68f7ae3713dd62b800518dffa7e605350b57780906669336d65cb0891f7615a17bd1c7db8dce8626498d336dacaa30497a3e54d44079f56584fdbdb8b783f8a11b89e4f9cc4aa920427bfda5958efa8dc0501b6ac9a678b0997cb778371fa75c0d336da9850cb1be692340727b2fd0955222796d6b0efae04091c4a42e4ebfef72aa4b2b22139f0db550c2f5c658b4cfd99e5b183af78a553eb41401b6a95a4d2671e3c90df7207eed832884fe58af714d481c3a84789f957602f8eef96a8302412b498db5cf2c626ef5dfe29a877f3ab601e91c5cacacf37217e5e918311b6b29334cf2b89e8e02cac8ba854d411be5c5e194cd33946dae3848e56d101e6ef47f3f7c046e523e9fd5548dfc8084a3b88e2c5617255e34cdb6a61332c7fb7a0de2f416bffbbe3c2cac12ae1962d3336d5330be7a3949d102385552528cdddf51101238b0adc9285d330bff1b6a658b4cfe50b67f0f61a8aa940462cac71bf64ae44ca2736d5ffc32c5a665d6b0bfaf489e490881e5aa6a086e73f939812099a666dacb963dd857b8511a2bfe70ebd075fa72406da994cd33958e17dd5298078e7e14456ee58e3d56771c229a887d2de7c202350909434cdb6a61332c6fbb70ffbe2c564ff3a882300cb16999b6a9985f74bfa7fb9595a9a843e406e89228b5eb875d8f4e4a169fcc1962d3336d5730be7ef741514bc71611ab7497e428a41bc6490c440381d063833c8b675f7267c6ba3fdb951dfa595bc95552378c9218880703a0c7067916cebee4cf8d747fb72a3bf4b2b792aaa46f192431100e07418e0cf22d9d7dc99f1ae8ff6e5477e9656f255548aa9384fd4008978266999b6b2e5231c21f16562de5f7a2eec8e8ff705b1c35297ae5fd02cae03cdc2e4860c465334ccdb596f9f83bf917407f3f3e52a96560fa88bd6f0a038242f4a34cdb6a4ccb1c2eed1ade82cace56be9fc237093b6fe90213a3f4a184ccb15e699b6d6ea22535149e440fa7a3b71cfa15d91c037169f8cb16999330bcdb5ba5ea486a10a27ab2ba55201bc8f00f932fba75a35d1fec63833ca5e5db851d46408519204a2eb585e4ca273be9ffd62b06dab9e0cb1699efef912aa4aa940462cac71bf64ae44ca2736d5ffc32c5a665d6b0bfaf489e49080 >'. (url=file:///home/blong/Dropbox/Public/example.mkv)

But the video works fine if I do:

gst-git gst-launch-1.0 playbin uri=file:///home/blong/Dropbox/Public/example.mkv
Comment 49 Philippe Normand 2013-06-26 00:21:21 PDT
(In reply to comment #48)
> I get this warning:
> 
> 0:00:00.142290527 29837      0x10da800 WARN       webkitmediaplayer /home/blong/workspace/webkit/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:726:handleMessage: Warning 6: No decoder available for type 'video/x-theora, width=(int)320, height=(int)240, 

Do you have theoradec ? I wonder why it's being plugged for a mkv file.
Comment 50 Brendan Long 2013-06-26 08:04:35 PDT
(In reply to comment #49)
> Do you have theoradec ? I wonder why it's being plugged for a mkv file.

Yes, I can gst-inspect-1.0 theoradec, and I can play that file with gst-launch-1.0 playbin uri=file:///path/to/file, plus it works if I build WebKit before these commits.

The file was created with GStreamer using something like this:

    gst-launch-1.0 -e videotestsrc ! theoraenc ! matroskamux name=mux ! filesink location=example.mkv videotestsrc pattern=checkers-8 ! theoraenc ! mux. audiotestsrc ! vorbisenc ! mux. audiotestsrc wave=white-noise ! vorbisenc ! mux.

The purpose is to have a file with multiple audio and video streams. So, the streams are theora.
Comment 51 Víctor M. Jáquez L. 2013-06-28 04:01:48 PDT
(In reply to comment #47)
> This patch causes certain videos to stop rendering for me using GStreamer master on Ubuntu 12.04 / Intel i7 graphics. Here's an example video:
> 
> https://www.dropbox.com/s/x03ogh3epbd6lfq/example.mkv
> 
> The audio plays but the video doesn't (and playbin never emits "video-changed").

I observe the same behavior in my setup. As if the video pad never got negotiated.
Comment 52 Sebastian Dröge (slomo) 2013-06-30 09:10:25 PDT
Can someone get a GStreamer debug log of this?
Comment 53 Víctor M. Jáquez L. 2013-07-01 04:18:20 PDT
Created attachment 205797 [details]
gstreamer log

It looks to me that the caps negotiation is not done correctly, since the webkit video sink never get _setcaps() called.
Comment 54 Sebastian Dröge (slomo) 2013-07-01 04:34:16 PDT
Uh, yes that looks like a really bad bug in playbin in 1.1.1/git master:

ed video/x-raw(meta:GstVideoGLTextureUploadMeta), format=(string){ BGRx, BGRA }, width=(int)[ 1, 2147483647 ], height=(int)[ 1, 2147483647 ], framerate=(fraction)[ 0/1, 2147483647/1 ]; video/x-surface, opengl=(boolean)true; video/x-raw, format=(string){ BGRx, BGRA }, width=(int)[ 1, 2147483647 ], height=(int)[ 1, 2147483647 ], framerate=(fraction)[ 0/1, 2147483647/1 ]
0:00:02.849152483 12953 0xae4022c0 DEBUG                playbin gstplaybin2.c:4302:autoplug_select_cb:<play> theoradec not compatible with the fixed sink
Comment 55 Sebastian Dröge (slomo) 2013-07-01 04:42:11 PDT
(In reply to comment #54)
> Uh, yes that looks like a really bad bug in playbin in 1.1.1/git master:
> 
> ed video/x-raw(meta:GstVideoGLTextureUploadMeta), format=(string){ BGRx, BGRA }, width=(int)[ 1, 2147483647 ], height=(int)[ 1, 2147483647 ], framerate=(fraction)[ 0/1, 2147483647/1 ]; video/x-surface, opengl=(boolean)true; video/x-raw, format=(string){ BGRx, BGRA }, width=(int)[ 1, 2147483647 ], height=(int)[ 1, 2147483647 ], framerate=(fraction)[ 0/1, 2147483647/1 ]
> 0:00:02.849152483 12953 0xae4022c0 DEBUG                playbin gstplaybin2.c:4302:autoplug_select_cb:<play> theoradec not compatible with the fixed sink

Also why does webkitvideosink still return video/x-surface? That should not be...
Comment 56 Sebastian Dröge (slomo) 2013-07-01 04:49:57 PDT
This probably fixes it: https://bugs.webkit.org/show_bug.cgi?id=116042


However really make sure that video/x-surface disappears from webkitvideosink. That's not how GStreamer is supposed to be used...
Comment 57 Sebastian Dröge (slomo) 2013-07-01 04:50:15 PDT
This probably fixes it: http://cgit.freedesktop.org/gstreamer/gst-plugins-base/commit/?id=f39d1dc3b48fad58668488ab985f10cca26478c7


However really make sure that video/x-surface disappears from webkitvideosink. That's not how GStreamer is supposed to be used...
Comment 58 Víctor M. Jáquez L. 2013-07-01 06:12:08 PDT
(In reply to comment #57)
> This probably fixes it: http://cgit.freedesktop.org/gstreamer/gst-plugins-base/commit/?id=f39d1dc3b48fad58668488ab985f10cca26478c7
> 
> 
> However really make sure that video/x-surface disappears from webkitvideosink. That's not how GStreamer is supposed to be used...

Yeah, I know, but I have it temporally for testing purposes.
Comment 59 Víctor M. Jáquez L. 2013-07-01 07:44:19 PDT
(In reply to comment #57)
> This probably fixes it: http://cgit.freedesktop.org/gstreamer/gst-plugins-base/commit/?id=f39d1dc3b48fad58668488ab985f10cca26478c7
> 

I've verified the fix: it works, thanks slomo!
Comment 60 Brendan Long 2013-07-01 09:03:00 PDT
(In reply to comment #57)

Works for me too. Thanks!