Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2127053004:
Optimize webgl texImage2D from YUV video textures. (Closed)
|
Created:
4 years, 5 months ago by jbauman Modified:
4 years, 5 months ago CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch+vc_chromium.org, miu+watch_chromium.org, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@skianocopy Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOptimize webgl texImage2D from YUV video textures.
The current path does a conversion from YUV to an RGBA SkImage, then
paints that to another Skia canvas and does a CopyTexImageCHROMIUM to
the WebGL texture.
We can optimize this by removing the paint to the canvas and doing a
copy directly from the SkImage to the WebGL texture.
BUG=625900
Committed: https://crrev.com/581d041c9971bb533e2cbd5c57014ee026fb50e6
Cr-Commit-Position: refs/heads/master@{#406735}
Patch Set 1 #
Total comments: 4
Patch Set 2 : hold on to converted SkImages #
Total comments: 3
Patch Set 3 : review changes #Patch Set 4 : remove unnecessary change #
Messages
Total messages: 26 (12 generated)
Description was changed from ========== Optimize webgl texImage2D from YUV video textures. The current path does a conversion from YUV to an RGBA SkImage, then paints that to another Skia canvas and does a CopyTexImageCHROMIUM to the WebGL texture. We can optimize this be removing the paint to the canvas and doing a copy directly from the SkImage to the WebGL texture. BUG=625900 ========== to ========== Optimize webgl texImage2D from YUV video textures. The current path does a conversion from YUV to an RGBA SkImage, then paints that to another Skia canvas and does a CopyTexImageCHROMIUM to the WebGL texture. We can optimize this by removing the paint to the canvas and doing a copy directly from the SkImage to the WebGL texture. BUG=625900 ==========
jbauman@chromium.org changed reviewers: + dcastagna@chromium.org, sandersd@chromium.org
dcastagna@chromium.org changed reviewers: + dongseong.hwang@intel.com
I have only one concern: IIUC if someone uploads a video with texImage2D multiple times, as it was before this patch, we'd cache the converted YUV videoframe and copy it in case they were trying to re-upload the same videoframe. After this patch we'd redo the conversion every time. Are we OK with that? Have you tried running the video-to-canvas perf tests? Adding DS that is familiar with the code. https://codereview.chromium.org/2127053004/diff/1/content/renderer/media/webm... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/2127053004/diff/1/content/renderer/media/webm... content/renderer/media/webmediaplayer_ms.cc:466: RenderThreadImpl::current()->SharedMainThreadContextProvider().get(); Isn't there a way to get a context provider without explicitly assuming which one it is? https://codereview.chromium.org/2127053004/diff/1/content/renderer/media/webm... content/renderer/media/webmediaplayer_ms.cc:472: return media::SkCanvasVideoRenderer::CopyVideoFrameTexturesToGLTexture( When is gl different than context_3d.gl? https://codereview.chromium.org/2127053004/diff/1/media/renderers/skcanvas_vi... File media/renderers/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/2127053004/diff/1/media/renderers/skcanvas_vi... media/renderers/skcanvas_video_renderer.cc:669: canvas_gl->ProduceTextureDirectCHROMIUM(texture_info->fID, Isn't this assuming that SkImage has only one texture? Right now we copy the three YUV texture into one when we create it, but in the future Skia might keep references to the three textures until it draws. https://codereview.chromium.org/2127053004/diff/1/media/renderers/skcanvas_vi... media/renderers/skcanvas_video_renderer.cc:685: destination_gl->CopyTextureCHROMIUM(source_texture2, texture, We're not validating internal_format anywhere if num_planes > 1. Should we?
On 2016/07/11 18:23:36, Daniele Castagna wrote: > I have only one concern: IIUC if someone uploads a video with texImage2D > multiple times, as it was before this patch, we'd cache the converted YUV > videoframe and copy it in case they were trying to re-upload the same > videoframe. After this patch we'd redo the conversion every time. > > Are we OK with that? Have you tried running the video-to-canvas perf tests? Ok, changed the patch to cache the last converted video frame so it'll avoid needing to reconvert it. This patch increases the performance on third_party/WebKit/PerformanceTests/Canvas/upload-video-to-texture.html from 650 runs/s to 1900 runs/s. > > Adding DS that is familiar with the code. > > https://codereview.chromium.org/2127053004/diff/1/content/renderer/media/webm... > File content/renderer/media/webmediaplayer_ms.cc (right): > > https://codereview.chromium.org/2127053004/diff/1/content/renderer/media/webm... > content/renderer/media/webmediaplayer_ms.cc:466: > RenderThreadImpl::current()->SharedMainThreadContextProvider().get(); > Isn't there a way to get a context provider without explicitly assuming which > one it is? Not really; there isn't that abstraction layer in content/renderer. This is the same way other code in WebMediaPlayerMS creates a context3d. > > https://codereview.chromium.org/2127053004/diff/1/content/renderer/media/webm... > content/renderer/media/webmediaplayer_ms.cc:472: return > media::SkCanvasVideoRenderer::CopyVideoFrameTexturesToGLTexture( > When is gl different than context_3d.gl? gl is the GL context for WebGL, while context_3d.gl is the GL context underneath skia's GrContext. > > https://codereview.chromium.org/2127053004/diff/1/media/renderers/skcanvas_vi... > File media/renderers/skcanvas_video_renderer.cc (right): > > https://codereview.chromium.org/2127053004/diff/1/media/renderers/skcanvas_vi... > media/renderers/skcanvas_video_renderer.cc:669: > canvas_gl->ProduceTextureDirectCHROMIUM(texture_info->fID, > Isn't this assuming that SkImage has only one texture? Right now we copy the > three YUV texture into one when we create it, but in the future Skia might keep > references to the three textures until it draws. Currently the interface for SkImage assumes there's only one texture. If we change that we can either make this code handle the conversion from YUV itself, or make it return false and let the caller handle the conversion. > > https://codereview.chromium.org/2127053004/diff/1/media/renderers/skcanvas_vi... > media/renderers/skcanvas_video_renderer.cc:685: > destination_gl->CopyTextureCHROMIUM(source_texture2, texture, > We're not validating internal_format anywhere if num_planes > 1. Should we? WebGLRenderingContextBase::texImageHelperHTMLVideoElement (the only thing that calls into it) validates that, as does the actual CopyTextureCHROMIM implementation. So I don't think we need validation here.
On 2016/07/12 at 04:17:48, jbauman wrote: > On 2016/07/11 18:23:36, Daniele Castagna wrote: > > I have only one concern: IIUC if someone uploads a video with texImage2D > > multiple times, as it was before this patch, we'd cache the converted YUV > > videoframe and copy it in case they were trying to re-upload the same > > videoframe. After this patch we'd redo the conversion every time. > > > > Are we OK with that? Have you tried running the video-to-canvas perf tests? > > Ok, changed the patch to cache the last converted video frame so it'll avoid needing to reconvert it. This patch increases the performance on third_party/WebKit/PerformanceTests/Canvas/upload-video-to-texture.html from 650 runs/s to 1900 runs/s. Nice. > > > > Adding DS that is familiar with the code. > > > > https://codereview.chromium.org/2127053004/diff/1/content/renderer/media/webm... > > File content/renderer/media/webmediaplayer_ms.cc (right): > > > > https://codereview.chromium.org/2127053004/diff/1/content/renderer/media/webm... > > content/renderer/media/webmediaplayer_ms.cc:466: > > RenderThreadImpl::current()->SharedMainThreadContextProvider().get(); > > Isn't there a way to get a context provider without explicitly assuming which > > one it is? > > Not really; there isn't that abstraction layer in content/renderer. This is the same way other code in WebMediaPlayerMS creates a context3d. > > > > https://codereview.chromium.org/2127053004/diff/1/content/renderer/media/webm... > > content/renderer/media/webmediaplayer_ms.cc:472: return > > media::SkCanvasVideoRenderer::CopyVideoFrameTexturesToGLTexture( > > When is gl different than context_3d.gl? > gl is the GL context for WebGL, while context_3d.gl is the GL context underneath skia's GrContext. > > > > > https://codereview.chromium.org/2127053004/diff/1/media/renderers/skcanvas_vi... > > File media/renderers/skcanvas_video_renderer.cc (right): > > > > https://codereview.chromium.org/2127053004/diff/1/media/renderers/skcanvas_vi... > > media/renderers/skcanvas_video_renderer.cc:669: > > canvas_gl->ProduceTextureDirectCHROMIUM(texture_info->fID, > > Isn't this assuming that SkImage has only one texture? Right now we copy the > > three YUV texture into one when we create it, but in the future Skia might keep > > references to the three textures until it draws. > > Currently the interface for SkImage assumes there's only one texture. If we change that we can either make this code handle the conversion from YUV itself, or make it return false and let the caller handle the conversion. > > > > https://codereview.chromium.org/2127053004/diff/1/media/renderers/skcanvas_vi... > > media/renderers/skcanvas_video_renderer.cc:685: > > destination_gl->CopyTextureCHROMIUM(source_texture2, texture, > > We're not validating internal_format anywhere if num_planes > 1. Should we? > > WebGLRenderingContextBase::texImageHelperHTMLVideoElement (the only thing that calls into it) validates that, as does the actual CopyTextureCHROMIM implementation. So I don't think we need validation here. https://codereview.chromium.org/2127053004/diff/20001/media/renderers/skcanva... File media/renderers/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/2127053004/diff/20001/media/renderers/skcanva... media/renderers/skcanvas_video_renderer.cc:697: uint32_t source_texture2 = destination_gl->CreateAndConsumeTextureCHROMIUM( nit: why 2? https://codereview.chromium.org/2127053004/diff/20001/media/renderers/skcanva... media/renderers/skcanvas_video_renderer.cc:705: // Wait for copy to complete before source texture destruction. By source texture you mean texture_info->fID, not source_texture2, right? Can't that be deleted at any time in canvas context as long as we already consumed the mailbox in destination context?
media/ lgtm https://codereview.chromium.org/2127053004/diff/20001/media/renderers/skcanva... File media/renderers/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/2127053004/diff/20001/media/renderers/skcanva... media/renderers/skcanvas_video_renderer.cc:673: if (!last_image_) Given this pattern is used by both call sites, perhaps UpdateLastImage() should return a bool?
PTAL On 2016/07/13 20:51:01, Daniele Castagna wrote: > https://codereview.chromium.org/2127053004/diff/20001/media/renderers/skcanva... > File media/renderers/skcanvas_video_renderer.cc (right): > > https://codereview.chromium.org/2127053004/diff/20001/media/renderers/skcanva... > media/renderers/skcanvas_video_renderer.cc:697: uint32_t source_texture2 = > destination_gl->CreateAndConsumeTextureCHROMIUM( > nit: why 2? Renamed. > > https://codereview.chromium.org/2127053004/diff/20001/media/renderers/skcanva... > media/renderers/skcanvas_video_renderer.cc:705: // Wait for copy to complete > before source texture destruction. > By source texture you mean texture_info->fID, not source_texture2, right? > > Can't that be deleted at any time in canvas context as long as we already > consumed the mailbox in destination context? Changed comment to say that this only needs to wait for the consume (though it happens to also wait for the copy). On 2016/07/18 23:46:57, sandersd wrote: > media/ lgtm > > https://codereview.chromium.org/2127053004/diff/20001/media/renderers/skcanva... > File media/renderers/skcanvas_video_renderer.cc (right): > > https://codereview.chromium.org/2127053004/diff/20001/media/renderers/skcanva... > media/renderers/skcanvas_video_renderer.cc:673: if (!last_image_) > Given this pattern is used by both call sites, perhaps UpdateLastImage() should > return a bool? Done.
lgtm
The CQ bit was checked by jbauman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sandersd@chromium.org Link to the patchset: https://codereview.chromium.org/2127053004/#ps40001 (title: "review changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by jbauman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcastagna@chromium.org, sandersd@chromium.org Link to the patchset: https://codereview.chromium.org/2127053004/#ps60001 (title: "remove unnecessary change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by jbauman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Optimize webgl texImage2D from YUV video textures. The current path does a conversion from YUV to an RGBA SkImage, then paints that to another Skia canvas and does a CopyTexImageCHROMIUM to the WebGL texture. We can optimize this by removing the paint to the canvas and doing a copy directly from the SkImage to the WebGL texture. BUG=625900 ========== to ========== Optimize webgl texImage2D from YUV video textures. The current path does a conversion from YUV to an RGBA SkImage, then paints that to another Skia canvas and does a CopyTexImageCHROMIUM to the WebGL texture. We can optimize this by removing the paint to the canvas and doing a copy directly from the SkImage to the WebGL texture. BUG=625900 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Optimize webgl texImage2D from YUV video textures. The current path does a conversion from YUV to an RGBA SkImage, then paints that to another Skia canvas and does a CopyTexImageCHROMIUM to the WebGL texture. We can optimize this by removing the paint to the canvas and doing a copy directly from the SkImage to the WebGL texture. BUG=625900 ========== to ========== Optimize webgl texImage2D from YUV video textures. The current path does a conversion from YUV to an RGBA SkImage, then paints that to another Skia canvas and does a CopyTexImageCHROMIUM to the WebGL texture. We can optimize this by removing the paint to the canvas and doing a copy directly from the SkImage to the WebGL texture. BUG=625900 Committed: https://crrev.com/581d041c9971bb533e2cbd5c57014ee026fb50e6 Cr-Commit-Position: refs/heads/master@{#406735} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/581d041c9971bb533e2cbd5c57014ee026fb50e6 Cr-Commit-Position: refs/heads/master@{#406735} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
