Bug 117551

Summary: [GStreamer] [texmap] the bytesPerLine is wrong when updating the texture
Product: WebKit Reporter: Víctor M. Jáquez L. <vjaquez>
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: calvaris, commit-queue, eric.carlson, glenn, gustavo, jer.noble, menard, mrobinson, pnormand, rego+ews, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 117736    
Bug Blocks: 105191    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Víctor M. Jáquez L. 2013-06-12 09:49:46 PDT
The bytesPerLine is wrong when updating the texture, because the naturalSize() is not updated to the current buffer size, so the stride might be wrong.
Comment 1 Víctor M. Jáquez L. 2013-06-12 09:54:30 PDT
Created attachment 204451 [details]
Patch
Comment 2 Martin Robinson 2013-06-12 10:28:34 PDT
Comment on attachment 204451 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:342
> +    GRefPtr<GstCaps> caps = webkitGstGetPadCaps(m_videoSinkPad.get());

Would it be better to get the caps before the buffer and pass that as an argument? Is there any way to lock the sink and get the buffer and the caps without the possibility of a race condition? This seems like a pretty serious issue.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:344
> +    GRefPtr<GstCaps> caps = GST_BUFFER_CAPS(m_buffer);

Any reason you are getting the cap from m_buffer instead of the argument, buffer?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:349
> +    int parN, parD, stride;

Please don't abbreviate variable names. :)
Comment 3 Víctor M. Jáquez L. 2013-06-12 10:54:18 PDT
Comment on attachment 204451 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:342
>> +    GRefPtr<GstCaps> caps = webkitGstGetPadCaps(m_videoSinkPad.get());
> 
> Would it be better to get the caps before the buffer and pass that as an argument? Is there any way to lock the sink and get the buffer and the caps without the possibility of a race condition? This seems like a pretty serious issue.

We could held a copy of the current caps in the sink.

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:344
>> +    GRefPtr<GstCaps> caps = GST_BUFFER_CAPS(m_buffer);
> 
> Any reason you are getting the cap from m_buffer instead of the argument, buffer?

oops! c&p error :$

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:349
>> +    int parN, parD, stride;
> 
> Please don't abbreviate variable names. :)

Ok! Thanks!
Comment 4 Martin Robinson 2013-06-12 11:05:36 PDT
(In reply to comment #3)
> (From update of attachment 204451 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=204451&action=review
> 
> >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:342
> >> +    GRefPtr<GstCaps> caps = webkitGstGetPadCaps(m_videoSinkPad.get());
> > 
> > Would it be better to get the caps before the buffer and pass that as an argument? Is there any way to lock the sink and get the buffer and the caps without the possibility of a race condition? This seems like a pretty serious issue.
> 
> We could held a copy of the current caps in the sink.

Maybe we can do it in this patch?
Comment 5 Víctor M. Jáquez L. 2013-06-12 11:16:51 PDT
Created attachment 204460 [details]
Patch
Comment 6 Víctor M. Jáquez L. 2013-06-12 11:22:43 PDT
Comment on attachment 204451 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:342
>>>> +    GRefPtr<GstCaps> caps = webkitGstGetPadCaps(m_videoSinkPad.get());
>>> 
>>> Would it be better to get the caps before the buffer and pass that as an argument? Is there any way to lock the sink and get the buffer and the caps without the possibility of a race condition? This seems like a pretty serious issue.
>> 
>> We could held a copy of the current caps in the sink.
> 
> Maybe we can do it in this patch?

I can do it in this patch, though it won't be related with this bug report...
Comment 7 Martin Robinson 2013-06-12 11:27:09 PDT
(In reply to comment #6)

> I can do it in this patch, though it won't be related with this bug report...


I'm not sure I understand, since the patch is introducing a potential race condition. Wouldn't it be better to write the patch without the race condition?
Comment 8 Xabier Rodríguez Calvar 2013-06-18 01:52:34 PDT
Víctor, when uploading a new patch, please remember to try media/video-display-aspect-ratio.html and if it works, please remove the line at LayoutTests/platform/gtk-wk2/TestExpectations.
Comment 9 Víctor M. Jáquez L. 2013-06-18 10:25:15 PDT
Created attachment 204922 [details]
Patch
Comment 10 Víctor M. Jáquez L. 2013-06-19 05:22:06 PDT
Created attachment 204993 [details]
Patch
Comment 11 Philippe Normand 2013-06-19 05:55:47 PDT
Comment on attachment 204993 [details]
Patch

Looks good, thanks!
Comment 12 WebKit Commit Bot 2013-06-19 08:04:19 PDT
Comment on attachment 204993 [details]
Patch

Clearing flags on attachment: 204993

Committed r151733: <http://trac.webkit.org/changeset/151733>
Comment 13 WebKit Commit Bot 2013-06-19 08:04:23 PDT
All reviewed patches have been landed.  Closing bug.