Render textures directly in VideoFrameChromium
Created attachment 69131 [details] Patch
Comment on attachment 69131 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69131&action=review What's the lifetime of the texture if it is provided by the frame? I'm curious how that is tied in to the lifetime of the compositor (specifically how long it is safe to stash it in a member of the layer). > WebCore/platform/graphics/chromium/VideoFrameChromium.h:66 > + TypeGlTexture, GlTexture is redundant - in WebCore the only type of texture is a GraphicsContext3D texture which is GL.
Created attachment 69137 [details] Patch
(In reply to comment #2) > (From update of attachment 69131 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=69131&action=review > > What's the lifetime of the texture if it is provided by the frame? I'm curious how that is tied in to the lifetime of the compositor (specifically how long it is safe to stash it in a member of the layer). The lifetime tires directly to HTMLMediaElement, it is no longer than the compositor. Since the lifetime of VideoLayerChromium tires to HTMLMediaElement, we should be safe to use the texture as long as VideoLayerChromium is alive. There might be some edge cases that we need to deal with later. However we don't keep the textures for a long period of time, we should keep the texture until swap buffers happen. > > > WebCore/platform/graphics/chromium/VideoFrameChromium.h:66 > > + TypeGlTexture, > > GlTexture is redundant - in WebCore the only type of texture is a GraphicsContext3D texture which is GL. Renamed it to TypeTexture.
(In reply to comment #4) > (In reply to comment #2) > > (From update of attachment 69131 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=69131&action=review > > > > What's the lifetime of the texture if it is provided by the frame? I'm curious how that is tied in to the lifetime of the compositor (specifically how long it is safe to stash it in a member of the layer). > > The lifetime tires directly to HTMLMediaElement, it is no longer than the compositor. Since the lifetime of VideoLayerChromium tires to HTMLMediaElement, we should be safe to use the texture as long as VideoLayerChromium is alive. There might be some edge cases that we need to deal with later. That's not true - layers outlive their associated elements.
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #2) > > > (From update of attachment 69131 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=69131&action=review > > > > > > What's the lifetime of the texture if it is provided by the frame? I'm curious how that is tied in to the lifetime of the compositor (specifically how long it is safe to stash it in a member of the layer). > > > > The lifetime tires directly to HTMLMediaElement, it is no longer than the compositor. Since the lifetime of VideoLayerChromium tires to HTMLMediaElement, we should be safe to use the texture as long as VideoLayerChromium is alive. There might be some edge cases that we need to deal with later. > > That's not true - layers outlive their associated elements. I see. In the case where we putVideoFrame() after swap buffers, what happens is that HTMLMediaElement will be destroyed, but the corresponding backend of media engine will still alive waiting for the video frame to be released. So we are safe to use the textures until we call putVideoFrame(). This patch need a later fix to hook to the swap buffers signal.
(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > (In reply to comment #2) > > > > (From update of attachment 69131 [details] [details] [details] [details]) > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=69131&action=review > > > > > > > > What's the lifetime of the texture if it is provided by the frame? I'm curious how that is tied in to the lifetime of the compositor (specifically how long it is safe to stash it in a member of the layer). > > > > > > The lifetime tires directly to HTMLMediaElement, it is no longer than the compositor. Since the lifetime of VideoLayerChromium tires to HTMLMediaElement, we should be safe to use the texture as long as VideoLayerChromium is alive. There might be some edge cases that we need to deal with later. > > > > That's not true - layers outlive their associated elements. > > I see. In the case where we putVideoFrame() after swap buffers, what happens is that HTMLMediaElement will be destroyed, but the corresponding backend of media engine will still alive waiting for the video frame to be released. So we are safe to use the textures until we call putVideoFrame(). This patch need a later fix to hook to the swap buffers signal. Oh and by the way, this code path will be exercised only behind a flag when hardware video decoding is enabled.
Comment on attachment 69137 [details] Patch If putCurrentFrame() can delete the texture then this will cause the compositor to attempt to use a texture after it's deleted since the draw call happens after updateContents. IMO this should be fixed before landing.
Created attachment 69284 [details] Patch
There are few main points in the latest patch: 1. Release the frame after the render calls if it contains textures 2. Add method to VideoLayerChromium so that it releases the frame if the owner of the frame is going away 3. VideoFrameProvider shouldn't be a OwnPtr in VideoLayerChromium because it is implemented by WebMediaPlayerClientImpl which is owned by MediaPlayer. I don't know why it didn't blow up before..
Attachment 69284 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4163030
(In reply to comment #11) > Attachment 69284 [details] did not build on chromium: > Build output: http://queues.webkit.org/results/4163030 Need to roll deps to chromium to pull in new code that implements that method.
Comment on attachment 69284 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69284&action=review > WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:404 > + for (unsigned plane = 0; plane < VideoFrameChromium::maxPlanes; plane++) { It seems that if frame->surfaceType() != VideoFrameChromium::TypeTexture then m_textures will contain ids of textures created by the compositor which you will need to delete before zero-ing them out.
Comment on attachment 69284 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69284&action=review >> WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:404 >> + for (unsigned plane = 0; plane < VideoFrameChromium::maxPlanes; plane++) { > > It seems that if frame->surfaceType() != VideoFrameChromium::TypeTexture then m_textures will contain ids of textures created by the compositor which you will need to delete before zero-ing them out. This method is called only in construct and in releaseCurrentFrame(). releaseCurrentFrame() will early return for the case of TypeTexture because a frame is never saved in this case.
Comment on attachment 69284 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69284&action=review >>> WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:404 >>> + for (unsigned plane = 0; plane < VideoFrameChromium::maxPlanes; plane++) { >> >> It seems that if frame->surfaceType() != VideoFrameChromium::TypeTexture then m_textures will contain ids of textures created by the compositor which you will need to delete before zero-ing them out. > > This method is called only in construct and in releaseCurrentFrame(). releaseCurrentFrame() will early return for the case of TypeTexture because a frame is never saved in this case. Ah, makes sense. I missed the early return part. Thanks for clarifying.
Comment on attachment 69284 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69284&action=review > WebCore/ChangeLog:4 > + Reviewed by NOBODY (OOPS!). > + This needs a one-line patch description (usually the bug title) and a link to the bug > WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:257 > + > + // FIXME: If surface type is GL texture we should call putCurrentFrame() only after swap buffers happened. > m_provider->putCurrentFrame(frame); I think this goes away now > WebKit/chromium/public/WebVideoFrame.h:56 > + SurfaceTypeGlTexture, This should probably match the other SurfaceType enum (i.e. not have 'Gl').
Created attachment 69531 [details] Patch
(In reply to comment #17) > Created an attachment (id=69531) [details] > Patch DEPS chromium has already been rolled forward so it's not necessary to do it in this patch.
Comment on attachment 69531 [details] Patch Looks good to me. Marking R+, although it would be great if Vangelis would give this a once-over before landing as well.
Comment on attachment 69531 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69531&action=review Only nits from my side. Otherwise looks good. > WebCore/platform/graphics/chromium/VideoLayerChromium.h:52 > + // This function is called by VideFrameProvider. When this method is called typo: VideoFrameProvider > WebKit/chromium/ChangeLog:8 > + Missing link to bug