Bug 117551 – [GStreamer] [texmap] the bytesPerLine is wrong when updating the texture

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Víctor M. Jáquez L.]

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.

[Víctor M. Jáquez L.]

Created attachment 204451 [details]
Patch

[Martin Robinson]

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. :)

[Víctor M. Jáquez L.]

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!

[Martin Robinson]

(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?

[Víctor M. Jáquez L.]

Created attachment 204460 [details]
Patch

[Víctor M. Jáquez L.]

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...

[Martin Robinson]

(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?

[Xabier Rodríguez Calvar]

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.

[Víctor M. Jáquez L.]

Created attachment 204922 [details]
Patch

[Víctor M. Jáquez L.]

Created attachment 204993 [details]
Patch

[Philippe Normand]

Comment on attachment 204993 [details]
Patch

Looks good, thanks!

[WebKit Commit Bot]

Comment on attachment 204993 [details]
Patch

Clearing flags on attachment: 204993

Committed r151733: <http://trac.webkit.org/changeset/151733>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.