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

Víctor M. Jáquez L.
Reported 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.
Attachments
Patch (2.75 KB, patch)
2013-06-12 09:54 PDT, Víctor M. Jáquez L.
no flags
Patch (2.84 KB, patch)
2013-06-12 11:16 PDT, Víctor M. Jáquez L.
no flags
Patch (4.61 KB, patch)
2013-06-18 10:25 PDT, Víctor M. Jáquez L.
no flags
Patch (4.56 KB, patch)
2013-06-19 05:22 PDT, Víctor M. Jáquez L.
no flags
Víctor M. Jáquez L.
Comment 1 2013-06-12 09:54:30 PDT
Martin Robinson
Comment 2 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. :)
Víctor M. Jáquez L.
Comment 3 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!
Martin Robinson
Comment 4 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?
Víctor M. Jáquez L.
Comment 5 2013-06-12 11:16:51 PDT
Víctor M. Jáquez L.
Comment 6 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...
Martin Robinson
Comment 7 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?
Xabier Rodríguez Calvar
Comment 8 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.
Víctor M. Jáquez L.
Comment 9 2013-06-18 10:25:15 PDT
Víctor M. Jáquez L.
Comment 10 2013-06-19 05:22:06 PDT
Philippe Normand
Comment 11 2013-06-19 05:55:47 PDT
Comment on attachment 204993 [details] Patch Looks good, thanks!
WebKit Commit Bot
Comment 12 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>
WebKit Commit Bot
Comment 13 2013-06-19 08:04:23 PDT
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.