Summary: | [GStreamer] A video element isn't drawn onto the canvas. | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Seongjun Kim <isair> | ||||||||||||||||
Component: | Media | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | agomez, calvaris, cgarcia, commit-queue, eric.carlson, glenn, gustavo, isair, jer.noble, menard, mrobinson, noam, philipj, pnormand, sergio, vjaquez, yoon | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Seongjun Kim
2014-05-07 22:16:31 PDT
Created attachment 231047 [details]
example
This is a regression of below. https://bugs.webkit.org/show_bug.cgi?id=86410 (WebCore::MediaPlayerPrivateGStreamerBase::paint) is used for 'drawImage'. But when accelerated compositing is used, this function didn't work. This exception for AC is still needed? Created attachment 231048 [details]
Patch
(In reply to comment #2) > This is a regression of below. > https://bugs.webkit.org/show_bug.cgi?id=86410 > > > (WebCore::MediaPlayerPrivateGStreamerBase::paint) is used for 'drawImage'. > But when accelerated compositing is used, this function didn't work. > > > This exception for AC is still needed? In AC the video is painted using GL textures. Look at void MediaPlayerPrivateGStreamerBase::paintToTextureMapper(TextureMapper* textureMapper, const FloatRect& targetRect, const TransformationMatrix& matrix, float opacity) If AC is not used, the painting is done through Cairo, and it's when this method is used. In AC we wait for the TextureMapper update to paint the video buffer in the texture. Removing this guard will mean to mix approaches, and that's not sane. (In reply to comment #4) > (In reply to comment #2) > > This is a regression of below. > > https://bugs.webkit.org/show_bug.cgi?id=86410 > > > > > > (WebCore::MediaPlayerPrivateGStreamerBase::paint) is used for 'drawImage'. > > But when accelerated compositing is used, this function didn't work. > > > > > > This exception for AC is still needed? > > In AC the video is painted using GL textures. Look at > > void MediaPlayerPrivateGStreamerBase::paintToTextureMapper(TextureMapper* textureMapper, const FloatRect& targetRect, const TransformationMatrix& matrix, float opacity) > > If AC is not used, the painting is done through Cairo, and it's when this method is used. > > In AC we wait for the TextureMapper update to paint the video buffer in the texture. > > Removing this guard will mean to mix approaches, and that's not sane. Did you mean that 'drawImage' should use 'paintToTextureMapper' instead of 'paint' in AC? Comment on attachment 231048 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231048&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:-350 > - return; What about, instead of removing this code, and instead or a simple return, we do if (supportsAcceleratedRendering() && m_player->mediaPlayerClient()->mediaPlayerRenderingCanBeAccelerated(m_player) && client()) { client()->setPlatformLayerNeedsDisplay(); return; } just as in void MediaPlayerPrivateGStreamerBase::triggerRepaint(GstBuffer* buffer) Comment on attachment 231048 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231048&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:-350 > - return; What about, instead of removing this code, and instead or a simple return, we do if (supportsAcceleratedRendering() && m_player->mediaPlayerClient()->mediaPlayerRenderingCanBeAccelerated(m_player) && client()) { client()->setPlatformLayerNeedsDisplay(); return; } just as in void MediaPlayerPrivateGStreamerBase::triggerRepaint(GstBuffer* buffer) Comment on attachment 231048 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231048&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:-350 > - return; What about, instead of removing this code, and instead or a simple return, we do if (supportsAcceleratedRendering() && m_player->mediaPlayerClient()->mediaPlayerRenderingCanBeAccelerated(m_player) && client()) { client()->setPlatformLayerNeedsDisplay(); return; } just as in void MediaPlayerPrivateGStreamerBase::triggerRepaint(GstBuffer* buffer) Comment on attachment 231048 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231048&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:-350 > - return; What about, instead of removing this code, and instead or a simple return, we do if (supportsAcceleratedRendering() && m_player->mediaPlayerClient()->mediaPlayerRenderingCanBeAccelerated(m_player) && client()) { client()->setPlatformLayerNeedsDisplay(); return; } just as in void MediaPlayerPrivateGStreamerBase::triggerRepaint(GstBuffer* buffer) Comment on attachment 231048 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231048&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:-350 > - return; What about, instead of removing this code, and instead or a simple return, we do if (supportsAcceleratedRendering() && m_player->mediaPlayerClient()->mediaPlayerRenderingCanBeAccelerated(m_player) && client()) { client()->setPlatformLayerNeedsDisplay(); return; } just as in void MediaPlayerPrivateGStreamerBase::triggerRepaint(GstBuffer* buffer) (In reply to comment #1) > Created an attachment (id=231047) [details] > example Thanks for the patch, it indeed works :) Can a layout test be unskipped thanks to the fix? (In reply to comment #4) > (In reply to comment #2) > > This is a regression of below. > > https://bugs.webkit.org/show_bug.cgi?id=86410 > > > > > > (WebCore::MediaPlayerPrivateGStreamerBase::paint) is used for 'drawImage'. > > But when accelerated compositing is used, this function didn't work. > > > > > > This exception for AC is still needed? > > In AC the video is painted using GL textures. Look at > > void MediaPlayerPrivateGStreamerBase::paintToTextureMapper(TextureMapper* textureMapper, const FloatRect& targetRect, const TransformationMatrix& matrix, float opacity) > > If AC is not used, the painting is done through Cairo, and it's when this method is used. > > In AC we wait for the TextureMapper update to paint the video buffer in the texture. > > Removing this guard will mean to mix approaches, and that's not sane. ::paint is no longer used indeed excepted in this case, whatever if AC is disabled or not. I think this patch is ok. (In reply to comment #11) > ::paint is no longer used indeed excepted in this case, whatever if AC is disabled or not. I think this patch is ok. But it makes me wonder, what if the negotiated buffer cannot download the raw image, relying in gst_video_gl_texture_upload_meta_upload(). Or perhaps in the future, when we will be able to negotiate buffers as EGLImages, how could Cairo handle it. Perhaps I'm not understanding the issue correctly, but I think that ::paint shall be handled by the texturemapper too. (In reply to comment #11) > Thanks for the patch, it indeed works :) Can a layout test be unskipped thanks to the fix? I tried layouttest, but there are many crashed tests with following message. Xlib: extension "NV-GLX" missing on display ":1". Would you mind helping for me? (In reply to comment #12) > (In reply to comment #11) > > ::paint is no longer used indeed excepted in this case, whatever if AC is disabled or not. I think this patch is ok. > > But it makes me wonder, what if the negotiated buffer cannot download the raw image, relying in gst_video_gl_texture_upload_meta_upload(). Or perhaps in the future, when we will be able to negotiate buffers as EGLImages, how could Cairo handle it. > > Perhaps I'm not understanding the issue correctly, but I think that ::paint shall be handled by the texturemapper too. I think we're going a bit off-topic here but if we add EGLImage support it will be a new memory in the buffer and it should be possible to do a zero-copy of the texture into a Cairo-GL surface (I think/hope :)) (In reply to comment #13) > (In reply to comment #11) > > Thanks for the patch, it indeed works :) Can a layout test be unskipped thanks to the fix? > > > I tried layouttest, but there are many crashed tests with following message. > Xlib: extension "NV-GLX" missing on display ":1". > > Would you mind helping for me? Ping me on IRC Ping? :) I reinstall ubuntu 14.04 however, I still fail to run layout-test with Xvfb errors... I don't know what is wrong. How about applying this patch first or someone continue this work? To work around Xvbf errors you can set the USE_NATIVE_XDISPLAY=1 env var when running the layout tests. Fortunately, Xvfb problem is sovled after rebasing latest revision! I am runing layout test now :) This is going to need more work when we switch to glimagesink for rendering. ::paint will need to handle the texture stored in the sample and somehow render it with Cairo, if that's possible. Worst case we'll need to download the bitmap data from the GPU to CPU :/ (In reply to comment #20) > This is going to need more work when we switch to glimagesink for rendering. > ::paint will need to handle the texture stored in the sample and somehow > render it with Cairo, if that's possible. > > Worst case we'll need to download the bitmap data from the GPU to CPU :/ I'm not sure it makes sense to use the GL path of GStreamer if accelerated compositing isn't enabled. One thing we can start considering though is to deprecate the non-accelerated path entirely. (In reply to comment #21) > (In reply to comment #20) > > This is going to need more work when we switch to glimagesink for rendering. > > ::paint will need to handle the texture stored in the sample and somehow > > render it with Cairo, if that's possible. > > > > Worst case we'll need to download the bitmap data from the GPU to CPU :/ > > I'm not sure it makes sense to use the GL path of GStreamer if accelerated > compositing isn't enabled. One thing we can start considering though is to > deprecate the non-accelerated path entirely. I agree, perhaps we should wait the gst 1.6 release though. Anyway for this specific issue it seems we could directly render the gst texture to a cairo-gl surface. Created attachment 249912 [details]
patch
Likely more iterations are needed, I'm not very familiar with the
GraphicsContext stuff :)
Comment on attachment 249912 [details]
patch
I think it is much safe to test ENABLE(ACCELERATED_2D_CANVAS) with a USE(CAIRO) to ensure we are using cairo-gl.
Created attachment 249918 [details]
patch
Comment on attachment 249918 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=249918&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:83 > +#if USE(CAIRO) > +#include <cairo-gl.h> > +#endif We can compile with cairo, but without cairo gl. We should expose those to the build somehow, or use ENABLE(ACCELERATED_2D_CANVAS) && USE(CAIRO) > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:579 > +PassNativeImagePtr MediaPlayerPrivateGStreamerBase::nativeImageForCurrentTime() > +{ > +#if !USE(GSTREAMER_GL) > + return nullptr; > +#endif This function is called inside a #if USE(GSTREAMER_GL) block, right? I think we could just defined it only in such case > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:595 > + if (!GST_IS_SAMPLE(m_sample.get())) > + return nullptr; > + > + GstCaps* caps = gst_sample_get_caps(m_sample.get()); > + if (!caps) > + return nullptr; > + > + GstVideoInfo videoInfo; > + gst_video_info_init(&videoInfo); > + if (!gst_video_info_from_caps(&videoInfo, caps)) > + return nullptr; It seems to me this code is duplicated in several places already, maybe we could move it to a helper function, something like bool getSampleVideoInfo(GstSmaple* sample, GstVideoInfo& videoInfo); > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:602 > +#if USE(CAIRO) Do we need previous stuff (gst_video_frame_map) when not using cairo? > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:604 > + GLContext::sharingContext()->makeContextCurrent(); > + GLContext* context = GLContext::sharingContext(); Why getting the sharing context twice? Can we get the context first and then calling context->makeContextCurrent() ? > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:611 > + FloatSize size = FloatSize(GST_VIDEO_INFO_WIDTH(&videoInfo), GST_VIDEO_INFO_HEIGHT(&videoInfo)); Why FloatSize? cairo expects integers Created attachment 249924 [details]
patch
Created attachment 249926 [details]
patch
Created attachment 249927 [details]
patch
Comment on attachment 249927 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=249927&action=review I only spot-checked this for obvious C++ errors, since not familiar with this code, but most of these changes only execute when two experimental build options are enabled (ACCELERATED_2D_CANVAS and USE_GSTREAMER_GL), neither of which we are likely to turn on for end users anytime soon, so it seems silly for this to be stalled on request queue any longer. > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:275 > + // The destination rectangle will have it's width and height already reversed for the orientation of it's -> its This one no longer works... I'll have to dive in again. (In reply to comment #31) > This one no longer works... I'll have to dive in again. I had the threaded compositor disabled, it works if I enable it. Will land the patch. Committed r194528: <http://trac.webkit.org/changeset/194528> |