Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1144323003:
media: Refactor SkCanvasVideoRenderer to use SkImages and not SkBitmaps. (Closed)
|
Created:
5 years, 7 months ago by Daniele Castagna Modified:
5 years, 4 months ago Reviewers:
reveman, dshwang, reed2, bsalomon_chromium, danakj, mcasas, bsalomon, DaleCurtis, reed1, scherkus (not reviewing) CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia: Refactor SkCanvasVideoRenderer to use SkImages and not SkBitmaps.
Refactor SkCanvasVideoRenderer to use SkImages instead of SkBitmaps as
suggested by the Skia team.
With this patch we remove any explicit reference to SkBitmaps in SkCanvasVideoRenderer.
The cache for the last frame is a SkImage that hides specific implementation
details.
This patch also avoids a copy when VideoFrame is backed by only one native
texture and it's possible to use it it directly to draw it to the canvas
instead of copying it to a temporary texture before drawing.
Additionally, a discardable memory allocator instance has now to be set in
cc main test suite since SkImage uses discardable memory and cc tests uses
SkCanvaSVideoRenderer.
BUG=449197
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/5bf77af1588dbd967ad909eebec2e380dcb669a6
Cr-Commit-Position: refs/heads/master@{#343272}
Patch Set 1 #Patch Set 2 : Initialize discardable memory allocator to cc_unittests. #Patch Set 3 : Only one texture_id. Comments. Unref SkImage. #Patch Set 4 : Put back a frame chache in case of YUV softare frame. #Patch Set 5 : Fix a leak. Return video_frame after drawing to the canvas. #Patch Set 6 : Remove the cache once again. Increases the capture buffer pool size. #Patch Set 7 : Add check we're not drawing on SkPicture. Remove comment leftover. #Patch Set 8 : Fix an incomplete comment. #
Total comments: 4
Patch Set 9 : Address reveman's comments. #
Total comments: 12
Patch Set 10 : Use skia::RefPtr. Log texture_target in the DCHECK. #
Total comments: 5
Patch Set 11 : Rebase on master. Re-add the cache. #
Total comments: 20
Patch Set 12 : Set GrBackendTextureDesc RenderTarget flag. #Patch Set 13 : Rebase on master. Use NewFromAdoptedTexture. #Patch Set 14 : Clear the cache after 3 seconds. #
Total comments: 24
Patch Set 15 : Move UpdateReleaseSyncPoint after drawing. #Patch Set 16 : Invalidate cache when switching hw/sw canvas. #
Total comments: 8
Patch Set 17 : Avoid caching when not necessary. Remove hw/sw cache invalidation. #
Total comments: 3
Patch Set 18 : Update syncpoint a the end of Paint. #
Total comments: 2
Patch Set 19 : Let VideoImageGenerator hold to the VideoFrame. #
Total comments: 8
Patch Set 20 : Comment on video_frame lifetime. DCHECK(!HasTextures) in VIG. #Patch Set 21 : s/scoped_refptr<VideoFrame>&/VideoFrame*/ where possible. #
Messages
Total messages: 103 (34 generated)
Patchset #2 (id:20001) has been deleted
Patchset #5 (id:100001) has been deleted
Patchset #6 (id:140001) has been deleted
dcastagna@chromium.org changed reviewers: + bsalomon@chromium.org, mcasas@chromium.org, reed@chromium.org
dcastagna@chromium.org changed reviewers: + reveman@chromium.org
dcastagna@chromium.org changed reviewers: + dalecurtis@chromium.org
+dalecurtis as the owner of media.
https://codereview.chromium.org/1144323003/diff/200001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/200001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:108: mailbox_holder.texture_target == GL_TEXTURE_RECTANGLE_ARB); GL_TEXTURE_EXTERNAL_OES? required for SurfaceTexture support on Android https://codereview.chromium.org/1144323003/diff/200001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:259: unsigned texture_id_ = 0; nit: texture_id
https://codereview.chromium.org/1144323003/diff/200001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/200001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:108: mailbox_holder.texture_target == GL_TEXTURE_RECTANGLE_ARB); On 2015/05/27 at 17:54:20, reveman wrote: > GL_TEXTURE_EXTERNAL_OES? required for SurfaceTexture support on Android Done. https://codereview.chromium.org/1144323003/diff/200001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:259: unsigned texture_id_ = 0; On 2015/05/27 at 17:54:20, reveman wrote: > nit: texture_id Done.
No idea about this code, so I defer to the skia folk and other reviewers. https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:109: mailbox_holder.texture_target == GL_TEXTURE_EXTERNAL_OES); Add << texture_target?
+dongseong.hwang who has worked on this stuff.
dongseong.hwang@intel.com changed reviewers: + dongseong.hwang@intel.com
Overall direction is good. However, I'm not sure if it's acceptable to remove the cache. +junov Could you compare the performance before and after using blink_perf.canvas? tools/perf/run_benchmark --browser=release blink_perf.canvas --story-filter=video --also-run-disabled-tests Currently blink_perf.canvas test has many trouble in low-end android device, so it's disabled. It's why you need --also-run-disabled-tests option. In addition, you need this CL also; https://codereview.chromium.org/1163573003/ Nexus 5 and Nexus 6 is fine with this perf test. I guess there is perf regression on all platforms due to removing cache mechanism. https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:113: mailbox_holder.texture_target, mailbox_holder.mailbox.name); Need to insert new sync point so that GpuVideoDecoder makes sure all client completes to use this mailbox before deleting this VideoFrame. It's why SkCanvasVideoRenderer::CopyVideoFrameTextureToGLTexture uses VideoFrame::UpdateReleaseSyncPoint api. IMO this fucntion should reuse CopyVideoFrameTextureToGLTexture(). https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:261: SkImage* target_frame = nullptr; use skia::RefPtr<SkImage> https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.h (left): https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.h:78: SkBitmap last_frame_; This cache optimization is gone. Canvas2D and WebGL can draws same video frame multiple times. After this CL, this class decodes the video frame or create the video texture every time. https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.h:87: SkBitmap accelerated_last_frame_; software canvas and accelerated canvas draws the same video frame multiple times. It's why software caches and gpu caches are kept separately. for example, ./blink/tools/run_layout_tests.py virtual/gpu/fast/canvas/yuv-video-on-accelerated-canvas.html After this CL, same SkImage deals with both software SkCanvas and ganesh SkCanvas. I'm not sure if it's ok.
On 2015/05/28 at 15:54:18, dongseong.hwang wrote: > Overall direction is good. However, I'm not sure if it's acceptable to remove the cache. +junov > The explicit cache in this file has been removed because Skia will take care of caching (or not, if we don't have enough memory). > Could you compare the performance before and after using blink_perf.canvas? > tools/perf/run_benchmark --browser=release blink_perf.canvas --story-filter=video --also-run-disabled-tests > If I didn't do anything wrong, there seems to be significant improvements for the three performance tests you suggested to run: draw-video-to-hw-accelerated-canvas-2d improved by 292%. upload-video-to-sub-texture improved by 48%. upload-video-to-texture improved by 3%. > Currently blink_perf.canvas test has many trouble in low-end android device, so it's disabled. It's why you need --also-run-disabled-tests option. In addition, you need this CL also; https://codereview.chromium.org/1163573003/ > > Nexus 5 and Nexus 6 is fine with this perf test. I guess there is perf regression on all platforms due to removing cache mechanism. > No, there is an improvement because the cache is still there as I explained before, and we're also avoiding one copy when there is a native texture in VideoFrame. > https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... > File media/blink/skcanvas_video_renderer.cc (right): > > https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... > media/blink/skcanvas_video_renderer.cc:113: mailbox_holder.texture_target, mailbox_holder.mailbox.name); > Need to insert new sync point so that GpuVideoDecoder makes sure all client completes to use this mailbox before deleting this VideoFrame. > It's why SkCanvasVideoRenderer::CopyVideoFrameTextureToGLTexture uses VideoFrame::UpdateReleaseSyncPoint api. > IMO this fucntion should reuse CopyVideoFrameTextureToGLTexture(). > > https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... > media/blink/skcanvas_video_renderer.cc:261: SkImage* target_frame = nullptr; > use skia::RefPtr<SkImage> > > https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... > File media/blink/skcanvas_video_renderer.h (left): > > https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... > media/blink/skcanvas_video_renderer.h:78: SkBitmap last_frame_; > This cache optimization is gone. > Canvas2D and WebGL can draws same video frame multiple times. After this CL, this class decodes the video frame or create the video texture every time. > > https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... > media/blink/skcanvas_video_renderer.h:87: SkBitmap accelerated_last_frame_; > software canvas and accelerated canvas draws the same video frame multiple times. It's why software caches and gpu caches are kept separately. > > for example, ./blink/tools/run_layout_tests.py virtual/gpu/fast/canvas/yuv-video-on-accelerated-canvas.html > > After this CL, same SkImage deals with both software SkCanvas and ganesh SkCanvas. I'm not sure if it's ok.
Thank you for the review! https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:109: mailbox_holder.texture_target == GL_TEXTURE_EXTERNAL_OES); On 2015/05/27 at 22:37:16, DaleCurtis wrote: > Add << texture_target? Done. https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:113: mailbox_holder.texture_target, mailbox_holder.mailbox.name); On 2015/05/28 at 15:54:17, dshwang wrote: > Need to insert new sync point so that GpuVideoDecoder makes sure all client completes to use this mailbox before deleting this VideoFrame. We keep a scoped_refptr to the VideoFrame, the GpuVideoDecoder will not get back this VideoFrame until we've completely used this mailbox. A syncpoint is not needed in this case. Btw, a syncpoint wouldn't be needed even if we were not keeping a ref to the videoframe. A syncpoint here would be needed only if there were the possibility that the storage for this texture could be reused somewhere else. > It's why SkCanvasVideoRenderer::CopyVideoFrameTextureToGLTexture uses VideoFrame::UpdateReleaseSyncPoint api. > IMO this fucntion should reuse CopyVideoFrameTextureToGLTexture(). Why would you want to copy the texture another time, when you can simply wrap it and draw it to a canvas without an additional copy? https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:261: SkImage* target_frame = nullptr; On 2015/05/28 at 15:54:17, dshwang wrote: > use skia::RefPtr<SkImage> Done. https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.h (left): https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.h:78: SkBitmap last_frame_; On 2015/05/28 at 15:54:18, dshwang wrote: > This cache optimization is gone. > Canvas2D and WebGL can draws same video frame multiple times. After this CL, this class decodes the video frame or create the video texture every time. Skia caches the result internally. The VideoFrame is decoded (if by the coded you mean YUV -> RGB when necessary) and then the RGBA cached inside Skia. In addition to that, Skia uses discardable memory and has a budget that would apply to this cache too. In this way we save memory when needed too. The texture is not created everytime. Skia has a pool of textures. https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.h:87: SkBitmap accelerated_last_frame_; On 2015/05/28 at 15:54:18, dshwang wrote: > software canvas and accelerated canvas draws the same video frame multiple times. It's why software caches and gpu caches are kept separately. > I don't know too much about the internal of Skia, but I don't see why they couldn't cache both in the software and gpu case. > for example, ./blink/tools/run_layout_tests.py virtual/gpu/fast/canvas/yuv-video-on-accelerated-canvas.html > > After this CL, same SkImage deals with both software SkCanvas and ganesh SkCanvas. I'm not sure if it's ok. That's not the case. We create a new SkImage every time. Skia internally could cache the two separately.
On 2015/05/28 22:45:20, Daniele Castagna wrote: > On 2015/05/28 at 15:54:18, dongseong.hwang wrote: > > Overall direction is good. However, I'm not sure if it's acceptable to remove > the cache. +junov > > > > The explicit cache in this file has been removed because Skia will take care of > caching (or not, if we don't have enough memory). Could you explain how skia caches software SkImage? I see that GPU SkImage is not needed to cache because we can use the mailbox texture directly. However, how SW SkImage is cached? When SkCanvasVideoRenderer::Paint() is called multiple time for the same video frame, code looks creating SkImage via VideoImageGenerator every time. > > Could you compare the performance before and after using blink_perf.canvas? > > tools/perf/run_benchmark --browser=release blink_perf.canvas > --story-filter=video --also-run-disabled-tests > > > > If I didn't do anything wrong, there seems to be significant improvements for > the three performance tests you suggested to run: > > draw-video-to-hw-accelerated-canvas-2d improved by 292%. > upload-video-to-sub-texture improved by 48%. > upload-video-to-texture improved by 3%. Only draw-video-to-hw-accelerated-canvas-2d is related to this CL. upload-video-to-* uses different path; CopyVideoFrameTextureToGLTexture() Which platform did you measure it? Which the format of VideoFrame is used, NATIVE_TEXTURE or not? draw-video-to-hw-accelerated-canvas-2d has different perf metric per platform because - Linux uses SW video and HW canvas - Android, ChromeOS uses HW video and HW canvas Need to verify below case also in terms of cache - SW video and SW canvas - HW video and SW canvas https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:113: mailbox_holder.texture_target, mailbox_holder.mailbox.name); On 2015/05/28 22:48:12, Daniele Castagna wrote: > On 2015/05/28 at 15:54:17, dshwang wrote: > > Need to insert new sync point so that GpuVideoDecoder makes sure all client > completes to use this mailbox before deleting this VideoFrame. > > We keep a scoped_refptr to the VideoFrame, the GpuVideoDecoder will not get back > this VideoFrame until we've completely used this mailbox. A syncpoint is not > needed in this case. That's true in Renderer, but it's not true in GPU process. How can GPU process know the mailbox is completely used? > Btw, a syncpoint wouldn't be needed even if we were not keeping a ref to the > videoframe. A syncpoint here would be needed only if there were the possibility > that the storage for this texture could be reused somewhere else. No, this code should insert new sync point. Let's assume following case 1. Context A of GpuVideoDecoder create new mailbox with new sync point 2. Context B in this code uses the mailbox after waiting the sync point 3. Context A deletes the mailbox. When Context A deletes the texture bound to mailbox in GPU process, Context A must make sure Context B is completed, but it's impossible without sync point. > > It's why SkCanvasVideoRenderer::CopyVideoFrameTextureToGLTexture uses > VideoFrame::UpdateReleaseSyncPoint api. > > IMO this fucntion should reuse CopyVideoFrameTextureToGLTexture(). > > Why would you want to copy the texture another time, when you can simply wrap it > and draw it to a canvas without an additional copy? I got it. Cool. https://codereview.chromium.org/1144323003/diff/80002/media/blink/skcanvas_vi... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/80002/media/blink/skcanvas_vi... media/blink/skcanvas_video_renderer.cc:109: mailbox_holder.texture_target == GL_TEXTURE_EXTERNAL_OES) Can SkImage wrap RECTANBLE_ARB and EXTERNAL_OES also? +reed
bsalomon@google.com changed reviewers: + bsalomon@google.com
https://codereview.chromium.org/1144323003/diff/80002/media/blink/skcanvas_vi... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/80002/media/blink/skcanvas_vi... media/blink/skcanvas_video_renderer.cc:321: // TODO(dcastagna): release the texture in a callback once Skia API exposes Why don't we go ahead and add this functionality to Skia? The current test in IsCanvasDrawingOnPicture is a bit of a hack and fragile.
https://codereview.chromium.org/1144323003/diff/80002/media/blink/skcanvas_vi... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/80002/media/blink/skcanvas_vi... media/blink/skcanvas_video_renderer.cc:109: mailbox_holder.texture_target == GL_TEXTURE_EXTERNAL_OES) On 2015/05/29 08:49:47, dshwang wrote: > Can SkImage wrap RECTANBLE_ARB and EXTERNAL_OES also? +reed skia doesn't support RECTANBLE_ARB and EXTERNAL_OES for SkImage. https://code.google.com/p/skia/issues/detail?id=3868 Those format texture should be copied to GL_TEXTURE_2D texture by CopyVideoFrameTextureToSkBitmapTexture
Patchset #12 (id:270001) has been deleted
Patchset #11 (id:250001) has been deleted
reed@google.com changed reviewers: + reed@google.com
can you remove the #include of SkGrPixelRef.h in skcanvas_video_renderer.cc ?
the video_renderer appears to have been copy/pasted into content/renderer/media/android/webmediaplayer_android.cc So that file has the same icky use of SkGrPixelRef. Can that file take your same fix?
Patchset #11 (id:290001) has been deleted
Patchset #11 (id:310001) has been deleted
Patchset #11 (id:330001) has been deleted
Patchset #11 (id:350001) has been deleted
On 2015/05/29 at 08:49:47, dongseong.hwang wrote: > On 2015/05/28 22:45:20, Daniele Castagna wrote: > > On 2015/05/28 at 15:54:18, dongseong.hwang wrote: > > > Overall direction is good. However, I'm not sure if it's acceptable to remove > > the cache. +junov > > > > > > > The explicit cache in this file has been removed because Skia will take care of > > caching (or not, if we don't have enough memory). > > Could you explain how skia caches software SkImage? > I see that GPU SkImage is not needed to cache because we can use the mailbox texture directly. > However, how SW SkImage is cached? When SkCanvasVideoRenderer::Paint() is called multiple time for the same video frame, code looks creating SkImage via VideoImageGenerator every time. > Investigated a little more. I thought the texture would be cached by Skia given the same generator. I was wrong. More info here: https://code.google.com/p/skia/issues/detail?id=3870 The cache has been re-added and now SkCanvasVideoRenderer keeps around the last generated image. > > > Could you compare the performance before and after using blink_perf.canvas? > > > tools/perf/run_benchmark --browser=release blink_perf.canvas > > --story-filter=video --also-run-disabled-tests > > > > > > > If I didn't do anything wrong, there seems to be significant improvements for > > the three performance tests you suggested to run: > > > > draw-video-to-hw-accelerated-canvas-2d improved by 292%. > > upload-video-to-sub-texture improved by 48%. > > upload-video-to-texture improved by 3%. > > Only draw-video-to-hw-accelerated-canvas-2d is related to this CL. upload-video-to-* uses different path; CopyVideoFrameTextureToGLTexture() > I wonder why upload-video-to-sub-texture seems to be affected too then. > Which platform did you measure it? Which the format of VideoFrame is used, NATIVE_TEXTURE or not? Measured it on Mac with a native texture. It was so much faster because it was skipping a copy. Surprisingly drawing with a texture_rectangle bound to a texture_2d was working without problem on a Macbook (visually it seemed correct too). We can't rely on that though. Brian is working on https://code.google.com/p/skia/issues/detail?id=3868. At least we know we can have a significant improvement once we have that. > draw-video-to-hw-accelerated-canvas-2d has different perf metric per platform because > - Linux uses SW video and HW canvas > - Android, ChromeOS uses HW video and HW canvas > Linux is still a little faster with this patch than it used to be. Android and ChromeOS should be OK. Keep in mind that soon we'll have videoframes backed by three YUV native textures. So I don't think we should worry to much about the software video frame with a HV canvas case since it will be really unlikely in the future. > Need to verify below case also in terms of cache > - SW video and SW canvas > - HW video and SW canvas > How can I test those? Just disabling GPU compositing? I'll look into that next. > https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... > File media/blink/skcanvas_video_renderer.cc (right): > > https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... > media/blink/skcanvas_video_renderer.cc:113: mailbox_holder.texture_target, mailbox_holder.mailbox.name); > On 2015/05/28 22:48:12, Daniele Castagna wrote: > > On 2015/05/28 at 15:54:17, dshwang wrote: > > > Need to insert new sync point so that GpuVideoDecoder makes sure all client > > completes to use this mailbox before deleting this VideoFrame. > > > > We keep a scoped_refptr to the VideoFrame, the GpuVideoDecoder will not get back > > this VideoFrame until we've completely used this mailbox. A syncpoint is not > > needed in this case. > > That's true in Renderer, but it's not true in GPU process. How can GPU process know the mailbox is completely used? > > > Btw, a syncpoint wouldn't be needed even if we were not keeping a ref to the > > videoframe. A syncpoint here would be needed only if there were the possibility > > that the storage for this texture could be reused somewhere else. > > No, this code should insert new sync point. > Let's assume following case > 1. Context A of GpuVideoDecoder create new mailbox with new sync point > 2. Context B in this code uses the mailbox after waiting the sync point > 3. Context A deletes the mailbox. > > When Context A deletes the texture bound to mailbox in GPU process, Context A must make sure Context B is completed, but it's impossible without sync point. > > > > It's why SkCanvasVideoRenderer::CopyVideoFrameTextureToGLTexture uses > > VideoFrame::UpdateReleaseSyncPoint api. > > > IMO this fucntion should reuse CopyVideoFrameTextureToGLTexture(). > > > > Why would you want to copy the texture another time, when you can simply wrap it > > and draw it to a canvas without an additional copy? > > I got it. Cool. > > https://codereview.chromium.org/1144323003/diff/80002/media/blink/skcanvas_vi... > File media/blink/skcanvas_video_renderer.cc (right): > > https://codereview.chromium.org/1144323003/diff/80002/media/blink/skcanvas_vi... > media/blink/skcanvas_video_renderer.cc:109: mailbox_holder.texture_target == GL_TEXTURE_EXTERNAL_OES) > Can SkImage wrap RECTANBLE_ARB and EXTERNAL_OES also? +reed
On 2015/06/04 at 20:51:37, reed wrote: > can you remove the #include of SkGrPixelRef.h in skcanvas_video_renderer.cc ? Done.
On 2015/06/04 at 21:04:29, reed wrote: > the video_renderer appears to have been copy/pasted into > > content/renderer/media/android/webmediaplayer_android.cc > > So that file has the same icky use of SkGrPixelRef. Can that file take your same fix? crrev.com/989493002 has been reverted, once it re-lands SkGrPixelRef should go away.
https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:113: mailbox_holder.texture_target, mailbox_holder.mailbox.name); On 2015/05/29 at 08:49:46, dshwang wrote: > On 2015/05/28 22:48:12, Daniele Castagna wrote: > > On 2015/05/28 at 15:54:17, dshwang wrote: > > > Need to insert new sync point so that GpuVideoDecoder makes sure all client > > completes to use this mailbox before deleting this VideoFrame. > > > > We keep a scoped_refptr to the VideoFrame, the GpuVideoDecoder will not get back > > this VideoFrame until we've completely used this mailbox. A syncpoint is not > > needed in this case. > > That's true in Renderer, but it's not true in GPU process. How can GPU process know the mailbox is completely used? > > > Btw, a syncpoint wouldn't be needed even if we were not keeping a ref to the > > videoframe. A syncpoint here would be needed only if there were the possibility > > that the storage for this texture could be reused somewhere else. > > No, this code should insert new sync point. > Let's assume following case > 1. Context A of GpuVideoDecoder create new mailbox with new sync point > 2. Context B in this code uses the mailbox after waiting the sync point > 3. Context A deletes the mailbox. > > When Context A deletes the texture bound to mailbox in GPU process, Context A must make sure Context B is completed, but it's impossible without sync point. > Context A can delete the texture name whenever it wants as long as Context B created one from the mailbox before that. Deleting the texture name doesn't delete the texture object. From the extension specification: "The texture object is deleted once all contexts have deleted the texture name associated with the texture object, and detached it from all framebuffer objects as well as texture unit bindings." Take a look at https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer... that deletes the texture after all the texturerefs have been deleted. Said so, we still need a syncpoint after we draw with the texture, since the texture object might be reused, and we need to make sure we finish drawing before reusing the texture storage. > > > It's why SkCanvasVideoRenderer::CopyVideoFrameTextureToGLTexture uses > > VideoFrame::UpdateReleaseSyncPoint api. > > > IMO this fucntion should reuse CopyVideoFrameTextureToGLTexture(). > > > > Why would you want to copy the texture another time, when you can simply wrap it > > and draw it to a canvas without an additional copy? > > I got it. Cool. https://codereview.chromium.org/1144323003/diff/80002/media/blink/skcanvas_vi... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/80002/media/blink/skcanvas_vi... media/blink/skcanvas_video_renderer.cc:109: mailbox_holder.texture_target == GL_TEXTURE_EXTERNAL_OES) On 2015/05/29 at 17:30:30, dshwang wrote: > On 2015/05/29 08:49:47, dshwang wrote: > > Can SkImage wrap RECTANBLE_ARB and EXTERNAL_OES also? +reed > > skia doesn't support RECTANBLE_ARB and EXTERNAL_OES for SkImage. > https://code.google.com/p/skia/issues/detail?id=3868 > Those format texture should be copied to GL_TEXTURE_2D texture by CopyVideoFrameTextureToSkBitmapTexture This happened to work for texture_rectangle. :/ Brian is working on supporting this in Skia in the right way. I put back the copies for rectangle and external_oes. We'll have a significant improvement once Brian implements the support for different texture targets. https://codereview.chromium.org/1144323003/diff/80002/media/blink/skcanvas_vi... media/blink/skcanvas_video_renderer.cc:321: // TODO(dcastagna): release the texture in a callback once Skia API exposes On 2015/05/29 at 14:49:26, bsalomon wrote: > Why don't we go ahead and add this functionality to Skia? The current test in IsCanvasDrawingOnPicture is a bit of a hack and fragile. With the last patch texture_id is kept around for one more frame, so it's a little bit less fragile. Happy to change it as soon as we have the functionality in Skia though.
https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:113: mailbox_holder.texture_target, mailbox_holder.mailbox.name); On 2015/05/29 at 08:49:46, dshwang wrote: > On 2015/05/28 22:48:12, Daniele Castagna wrote: > > On 2015/05/28 at 15:54:17, dshwang wrote: > > > Need to insert new sync point so that GpuVideoDecoder makes sure all client > > completes to use this mailbox before deleting this VideoFrame. > > > > We keep a scoped_refptr to the VideoFrame, the GpuVideoDecoder will not get back > > this VideoFrame until we've completely used this mailbox. A syncpoint is not > > needed in this case. > > That's true in Renderer, but it's not true in GPU process. How can GPU process know the mailbox is completely used? > > > Btw, a syncpoint wouldn't be needed even if we were not keeping a ref to the > > videoframe. A syncpoint here would be needed only if there were the possibility > > that the storage for this texture could be reused somewhere else. > > No, this code should insert new sync point. > Let's assume following case > 1. Context A of GpuVideoDecoder create new mailbox with new sync point > 2. Context B in this code uses the mailbox after waiting the sync point > 3. Context A deletes the mailbox. > > When Context A deletes the texture bound to mailbox in GPU process, Context A must make sure Context B is completed, but it's impossible without sync point. > Context A can delete the texture name whenever it wants as long as Context B created one from the mailbox before that. Deleting the texture name doesn't delete the texture object. From the extension specification: "The texture object is deleted once all contexts have deleted the texture name associated with the texture object, and detached it from all framebuffer objects as well as texture unit bindings." Take a look at https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer... that deletes the texture after all the texturerefs have been deleted. Said so, we still need a syncpoint after we draw with the texture, since the texture object might be reused, and we need to make sure we finish drawing before reusing the texture storage. > > > It's why SkCanvasVideoRenderer::CopyVideoFrameTextureToGLTexture uses > > VideoFrame::UpdateReleaseSyncPoint api. > > > IMO this fucntion should reuse CopyVideoFrameTextureToGLTexture(). > > > > Why would you want to copy the texture another time, when you can simply wrap it > > and draw it to a canvas without an additional copy? > > I got it. Cool. https://codereview.chromium.org/1144323003/diff/80002/media/blink/skcanvas_vi... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/80002/media/blink/skcanvas_vi... media/blink/skcanvas_video_renderer.cc:109: mailbox_holder.texture_target == GL_TEXTURE_EXTERNAL_OES) On 2015/05/29 at 17:30:30, dshwang wrote: > On 2015/05/29 08:49:47, dshwang wrote: > > Can SkImage wrap RECTANBLE_ARB and EXTERNAL_OES also? +reed > > skia doesn't support RECTANBLE_ARB and EXTERNAL_OES for SkImage. > https://code.google.com/p/skia/issues/detail?id=3868 > Those format texture should be copied to GL_TEXTURE_2D texture by CopyVideoFrameTextureToSkBitmapTexture This happened to work for texture_rectangle. :/ Brian is working on supporting this in Skia in the right way. I put back the copies for rectangle and external_oes. We'll have a significant improvement once Brian implements the support for different texture targets. https://codereview.chromium.org/1144323003/diff/80002/media/blink/skcanvas_vi... media/blink/skcanvas_video_renderer.cc:321: // TODO(dcastagna): release the texture in a callback once Skia API exposes On 2015/05/29 at 14:49:26, bsalomon wrote: > Why don't we go ahead and add this functionality to Skia? The current test in IsCanvasDrawingOnPicture is a bit of a hack and fragile. With the last patch texture_id is kept around for one more frame, so it's a little bit less fragile. Happy to change it as soon as we have the functionality in Skia though.
> > Need to verify below case also in terms of cache > > - SW video and SW canvas > > - HW video and SW canvas > > > > How can I test those? Just disabling GPU compositing? I'll look into that next. via hardcoding in Blink.. On 2015/06/05 04:15:31, Daniele Castagna wrote: > https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... > File media/blink/skcanvas_video_renderer.cc (right): > > https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... > media/blink/skcanvas_video_renderer.cc:113: mailbox_holder.texture_target, > mailbox_holder.mailbox.name); > On 2015/05/29 at 08:49:46, dshwang wrote: > > On 2015/05/28 22:48:12, Daniele Castagna wrote: > > > On 2015/05/28 at 15:54:17, dshwang wrote: > > > > Need to insert new sync point so that GpuVideoDecoder makes sure all > client > > > completes to use this mailbox before deleting this VideoFrame. > > > > > > We keep a scoped_refptr to the VideoFrame, the GpuVideoDecoder will not get > back > > > this VideoFrame until we've completely used this mailbox. A syncpoint is not > > > needed in this case. > > > > That's true in Renderer, but it's not true in GPU process. How can GPU process > know the mailbox is completely used? > > > > > Btw, a syncpoint wouldn't be needed even if we were not keeping a ref to the > > > videoframe. A syncpoint here would be needed only if there were the > possibility > > > that the storage for this texture could be reused somewhere else. > > > > No, this code should insert new sync point. > > Let's assume following case > > 1. Context A of GpuVideoDecoder create new mailbox with new sync point > > 2. Context B in this code uses the mailbox after waiting the sync point > > 3. Context A deletes the mailbox. > > > > When Context A deletes the texture bound to mailbox in GPU process, Context A > must make sure Context B is completed, but it's impossible without sync point. > > > > Context A can delete the texture name whenever it wants as long as Context B > created one from the mailbox before that. Deleting the texture name doesn't > delete the texture object. > From the extension specification: > "The texture object is deleted once all contexts have deleted the texture > name > associated with the texture object, and detached it from all framebuffer > objects as well as texture unit bindings." > > Take a look at > https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer... > that deletes the texture after all the texturerefs have been deleted. > > Said so, we still need a syncpoint after we draw with the texture, since the > texture object might be reused, and we need to make sure we finish drawing > before reusing the texture storage. How can you make sure "Context B created one from the mailbox before that" without inserting new sync point?
On 2015/06/05 at 14:54:14, dongseong.hwang wrote: > > > Need to verify below case also in terms of cache > > > - SW video and SW canvas > > > - HW video and SW canvas > > > > > > > How can I test those? Just disabling GPU compositing? I'll look into that next. > > via hardcoding in Blink.. > > > On 2015/06/05 04:15:31, Daniele Castagna wrote: > > https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... > > File media/blink/skcanvas_video_renderer.cc (right): > > > > https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... > > media/blink/skcanvas_video_renderer.cc:113: mailbox_holder.texture_target, > > mailbox_holder.mailbox.name); > > On 2015/05/29 at 08:49:46, dshwang wrote: > > > On 2015/05/28 22:48:12, Daniele Castagna wrote: > > > > On 2015/05/28 at 15:54:17, dshwang wrote: > > > > > Need to insert new sync point so that GpuVideoDecoder makes sure all > > client > > > > completes to use this mailbox before deleting this VideoFrame. > > > > > > > > We keep a scoped_refptr to the VideoFrame, the GpuVideoDecoder will not get > > back > > > > this VideoFrame until we've completely used this mailbox. A syncpoint is not > > > > needed in this case. > > > > > > That's true in Renderer, but it's not true in GPU process. How can GPU process > > know the mailbox is completely used? > > > > > > > Btw, a syncpoint wouldn't be needed even if we were not keeping a ref to the > > > > videoframe. A syncpoint here would be needed only if there were the > > possibility > > > > that the storage for this texture could be reused somewhere else. > > > > > > No, this code should insert new sync point. > > > Let's assume following case > > > 1. Context A of GpuVideoDecoder create new mailbox with new sync point > > > 2. Context B in this code uses the mailbox after waiting the sync point > > > 3. Context A deletes the mailbox. > > > > > > When Context A deletes the texture bound to mailbox in GPU process, Context A > > must make sure Context B is completed, but it's impossible without sync point. > > > > > > > Context A can delete the texture name whenever it wants as long as Context B > > created one from the mailbox before that. Deleting the texture name doesn't > > delete the texture object. > > From the extension specification: > > "The texture object is deleted once all contexts have deleted the texture > > name > > associated with the texture object, and detached it from all framebuffer > > objects as well as texture unit bindings." > > > > Take a look at > > https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer... > > that deletes the texture after all the texturerefs have been deleted. > > > > Said so, we still need a syncpoint after we draw with the texture, since the > > texture object might be reused, and we need to make sure we finish drawing > > before reusing the texture storage. > > How can you make sure "Context B created one from the mailbox before that" without inserting new sync point? The texture name in Context A is not deleted until you keep the videoframe around.
On 2015/06/05 14:57:56, Daniele Castagna wrote: > On 2015/06/05 at 14:54:14, dongseong.hwang wrote: > > > > Need to verify below case also in terms of cache > > > > - SW video and SW canvas > > > > - HW video and SW canvas > > > > > > > > > > How can I test those? Just disabling GPU compositing? I'll look into that > next. > > > > via hardcoding in Blink.. > > > > > > On 2015/06/05 04:15:31, Daniele Castagna wrote: > > > > https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... > > > File media/blink/skcanvas_video_renderer.cc (right): > > > > > > > https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... > > > media/blink/skcanvas_video_renderer.cc:113: mailbox_holder.texture_target, > > > mailbox_holder.mailbox.name); > > > On 2015/05/29 at 08:49:46, dshwang wrote: > > > > On 2015/05/28 22:48:12, Daniele Castagna wrote: > > > > > On 2015/05/28 at 15:54:17, dshwang wrote: > > > > > > Need to insert new sync point so that GpuVideoDecoder makes sure all > > > client > > > > > completes to use this mailbox before deleting this VideoFrame. > > > > > > > > > > We keep a scoped_refptr to the VideoFrame, the GpuVideoDecoder will not > get > > > back > > > > > this VideoFrame until we've completely used this mailbox. A syncpoint is > not > > > > > needed in this case. > > > > > > > > That's true in Renderer, but it's not true in GPU process. How can GPU > process > > > know the mailbox is completely used? > > > > > > > > > Btw, a syncpoint wouldn't be needed even if we were not keeping a ref to > the > > > > > videoframe. A syncpoint here would be needed only if there were the > > > possibility > > > > > that the storage for this texture could be reused somewhere else. > > > > > > > > No, this code should insert new sync point. > > > > Let's assume following case > > > > 1. Context A of GpuVideoDecoder create new mailbox with new sync point > > > > 2. Context B in this code uses the mailbox after waiting the sync point > > > > 3. Context A deletes the mailbox. > > > > > > > > When Context A deletes the texture bound to mailbox in GPU process, > Context A > > > must make sure Context B is completed, but it's impossible without sync > point. > > > > > > > > > > Context A can delete the texture name whenever it wants as long as Context B > > > created one from the mailbox before that. Deleting the texture name doesn't > > > delete the texture object. > > > From the extension specification: > > > "The texture object is deleted once all contexts have deleted the texture > > > name > > > associated with the texture object, and detached it from all framebuffer > > > objects as well as texture unit bindings." > > > > > > Take a look at > > > > https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer... > > > that deletes the texture after all the texturerefs have been deleted. > > > > > > Said so, we still need a syncpoint after we draw with the texture, since the > > > texture object might be reused, and we need to make sure we finish drawing > > > before reusing the texture storage. > > > > How can you make sure "Context B created one from the mailbox before that" > without inserting new sync point? > > The texture name in Context A is not deleted until you keep the videoframe > around. I presented following example. 1. Context A of GpuVideoDecoder create new mailbox with new sync point 2. Context B in this code uses the mailbox after waiting the sync point 3. Context A deletes the mailbox. In GPU process, Context A can delete the mailbox before Context B creates the texture from the mailbox. It's not related to videoframe. No matter renderer holding videoframe, GPU process can re-order all gpu commands. only sync point can make sure the order.
On 2015/06/05 at 17:00:06, dongseong.hwang wrote: > On 2015/06/05 14:57:56, Daniele Castagna wrote: > > On 2015/06/05 at 14:54:14, dongseong.hwang wrote: > > > > > Need to verify below case also in terms of cache > > > > > - SW video and SW canvas > > > > > - HW video and SW canvas > > > > > > > > > > > > > How can I test those? Just disabling GPU compositing? I'll look into that > > next. > > > > > > via hardcoding in Blink.. > > > > > > > > > On 2015/06/05 04:15:31, Daniele Castagna wrote: > > > > > > https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... > > > > File media/blink/skcanvas_video_renderer.cc (right): > > > > > > > > > > https://codereview.chromium.org/1144323003/diff/220001/media/blink/skcanvas_v... > > > > media/blink/skcanvas_video_renderer.cc:113: mailbox_holder.texture_target, > > > > mailbox_holder.mailbox.name); > > > > On 2015/05/29 at 08:49:46, dshwang wrote: > > > > > On 2015/05/28 22:48:12, Daniele Castagna wrote: > > > > > > On 2015/05/28 at 15:54:17, dshwang wrote: > > > > > > > Need to insert new sync point so that GpuVideoDecoder makes sure all > > > > client > > > > > > completes to use this mailbox before deleting this VideoFrame. > > > > > > > > > > > > We keep a scoped_refptr to the VideoFrame, the GpuVideoDecoder will not > > get > > > > back > > > > > > this VideoFrame until we've completely used this mailbox. A syncpoint is > > not > > > > > > needed in this case. > > > > > > > > > > That's true in Renderer, but it's not true in GPU process. How can GPU > > process > > > > know the mailbox is completely used? > > > > > > > > > > > Btw, a syncpoint wouldn't be needed even if we were not keeping a ref to > > the > > > > > > videoframe. A syncpoint here would be needed only if there were the > > > > possibility > > > > > > that the storage for this texture could be reused somewhere else. > > > > > > > > > > No, this code should insert new sync point. > > > > > Let's assume following case > > > > > 1. Context A of GpuVideoDecoder create new mailbox with new sync point > > > > > 2. Context B in this code uses the mailbox after waiting the sync point > > > > > 3. Context A deletes the mailbox. > > > > > > > > > > When Context A deletes the texture bound to mailbox in GPU process, > > Context A > > > > must make sure Context B is completed, but it's impossible without sync > > point. > > > > > > > > > > > > > Context A can delete the texture name whenever it wants as long as Context B > > > > created one from the mailbox before that. Deleting the texture name doesn't > > > > delete the texture object. > > > > From the extension specification: > > > > "The texture object is deleted once all contexts have deleted the texture > > > > name > > > > associated with the texture object, and detached it from all framebuffer > > > > objects as well as texture unit bindings." > > > > > > > > Take a look at > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer... > > > > that deletes the texture after all the texturerefs have been deleted. > > > > > > > > Said so, we still need a syncpoint after we draw with the texture, since the > > > > texture object might be reused, and we need to make sure we finish drawing > > > > before reusing the texture storage. > > > > > > How can you make sure "Context B created one from the mailbox before that" > > without inserting new sync point? > > > > The texture name in Context A is not deleted until you keep the videoframe > > around. > > I presented following example. > > 1. Context A of GpuVideoDecoder create new mailbox with new sync point > 2. Context B in this code uses the mailbox after waiting the sync point > 3. Context A deletes the mailbox. > > In GPU process, Context A can delete the mailbox before Context B creates the texture from the mailbox. > It's not related to videoframe. No matter renderer holding videoframe, GPU process can re-order all gpu commands. only sync point can make sure the order. We can continue discussing this somewhere else if you want. If you look at the code in this patch a syncpoint is inserted in the VideoFrame after we're done drawing. So that will guarantee at least what you are concerned about.
https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:93: mailbox_holder.texture_target, mailbox_holder.mailbox.name); Following two lines is needed to insert sync point here. SyncPointClientImpl client(gl); video_frame->UpdateReleaseSyncPoint(&client);
https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (left): https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:235: frame_deleting_timer_( I think it's needed. e.g. if some canvas draws video and then doesn't draw anymore, we want to delete cache earlier than video life cycle. other example, if user switches tab, we want to delete cache soon. This caches can consumes few MB of GPU memory. https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:93: mailbox_holder.texture_target, mailbox_holder.mailbox.name); On 2015/06/05 18:58:16, dshwang wrote: > Following two lines is needed to insert sync point here. > > SyncPointClientImpl client(gl); > video_frame->UpdateReleaseSyncPoint(&client); ah, I found you insert sync point at the end of drawing. sorry for noise.
https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (left): https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:235: frame_deleting_timer_( On 2015/06/05 at 19:08:41, dshwang wrote: > I think it's needed. > e.g. if some canvas draws video and then doesn't draw anymore, we want to delete cache earlier than video life cycle. > other example, if user switches tab, we want to delete cache soon. > This caches can consumes few MB of GPU memory. SkImage caches the result internally and I'd assume it purges the cache when it's out of budget. |last_texture_id_| unfortunately takes GPU memory keeping the texture object alive. That will go away as soon as we have Skia support for different texture targets. At that point we won't need to copy the texture and we'll be able to get rid of |last_texture_id_|. Would you still suggest to leave that 3 second timer to reset the cache? Why was that added in the first place?
sorry for delaying reviewing. https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (left): https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:235: frame_deleting_timer_( On 2015/06/05 22:05:32, Daniele Castagna wrote: > On 2015/06/05 at 19:08:41, dshwang wrote: > > I think it's needed. > > e.g. if some canvas draws video and then doesn't draw anymore, we want to > delete cache earlier than video life cycle. > > other example, if user switches tab, we want to delete cache soon. > > This caches can consumes few MB of GPU memory. > > SkImage caches the result internally and I'd assume it purges the cache when > it's out of budget. > > |last_texture_id_| unfortunately takes GPU memory keeping the texture object > alive. That will go away as soon as we have Skia support for different texture > targets. At that point we won't need to copy the texture and we'll be able to > get rid of |last_texture_id_|. > > Would you still suggest to leave that 3 second timer to reset the cache? Why was > that added in the first place? 3 second timer was introduced to eagerly remove cache to not wait when it's out of budget. I think it's needed, but I'm not strongly claiming. DaleCurtis, mcasas, what do you think? https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:349: video_frame, last_frame_.getPixels(), last_frame_.rowBytes()); does VideoGenerator way have same performance to create SkBitmap explicitly? +bsalomon? https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:93: mailbox_holder.texture_target, mailbox_holder.mailbox.name); In this case, this class should not cache the texture. Gpu decoder can reuse the texture after VideoFrame destruction. Keeping this texture might cause data race. In addition, cache is meaningless in this case, because there is no copy. On the other hand, it's fine to cache the copied texture. https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:318: video_frame->UpdateReleaseSyncPoint(&client); It's needed only for GL_TEXTURE_2D case. https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.h (right): https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.h:78: base::TimeDelta last_timestamp_ = media::kNoTimestamp(); is it allowed to initialize here in chromium coding style? https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.h:90: gpu::gles2::GLES2Interface* last_gl_ = nullptr; it's very unsafe. it can easily cause sef fault. If you want to keep texture, keep GrTexture. GrTexture is deleted automatically when gl context is gone.
Patchset #13 (id:410001) has been deleted
Patchset #13 (id:430001) has been deleted
Patchset #13 (id:450001) has been deleted
https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (left): https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:235: frame_deleting_timer_( On 2015/06/08 at 12:21:43, dshwang wrote: > On 2015/06/05 22:05:32, Daniele Castagna wrote: > > On 2015/06/05 at 19:08:41, dshwang wrote: > > > I think it's needed. > > > e.g. if some canvas draws video and then doesn't draw anymore, we want to > > delete cache earlier than video life cycle. > > > other example, if user switches tab, we want to delete cache soon. > > > This caches can consumes few MB of GPU memory. > > > > SkImage caches the result internally and I'd assume it purges the cache when > > it's out of budget. > > > > |last_texture_id_| unfortunately takes GPU memory keeping the texture object > > alive. That will go away as soon as we have Skia support for different texture > > targets. At that point we won't need to copy the texture and we'll be able to > > get rid of |last_texture_id_|. > > > > Would you still suggest to leave that 3 second timer to reset the cache? Why was > > that added in the first place? > > 3 second timer was introduced to eagerly remove cache to not wait when > it's out of budget. > I think it's needed, but I'm not strongly claiming. DaleCurtis, mcasas, what do you think? My rationale for removing the delaytimer is that now we have a plan to get rid of the cache completely. https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:93: mailbox_holder.texture_target, mailbox_holder.mailbox.name); On 2015/06/08 at 12:21:43, dshwang wrote: > In this case, this class should not cache the texture. Gpu decoder can reuse the texture after VideoFrame destruction. Keeping this texture might cause data race. > In addition, cache is meaningless in this case, because there is no copy. > The lifetime of the texture id is currently not well defined since SkImage does not take the ownership of it. Let's discuss this once we fix that. https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:318: video_frame->UpdateReleaseSyncPoint(&client); On 2015/06/08 at 12:21:43, dshwang wrote: > It's needed only for GL_TEXTURE_2D case. That's true, but considering that eventually it will be needed for all the texture target, I'd prefer not to add specific cases to improve performance for a code path we're planning to remove. https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.h (right): https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.h:78: base::TimeDelta last_timestamp_ = media::kNoTimestamp(); On 2015/06/08 at 12:21:44, dshwang wrote: > is it allowed to initialize here in chromium coding style? It seems so, look for "Non-Static Class Member Initializers" here: https://chromium-cpp.appspot.com/ https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.h:90: gpu::gles2::GLES2Interface* last_gl_ = nullptr; On 2015/06/08 at 12:21:44, dshwang wrote: > it's very unsafe. it can easily cause sef fault. If you want to keep texture, keep GrTexture. GrTexture is deleted automatically when gl context is gone. Agreed. The idea in this CL is to use SkImage, so I'm not sure using a GrTexture would be aligned with Skia's goal. Let me discuss with the Skia team how we can fix this. It'd be nice if SkImage::NewFromTexture could take ownership of the backend texture and delete it when done.
https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (left): https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:235: frame_deleting_timer_( On 2015/06/09 23:21:11, Daniele Castagna wrote: > On 2015/06/08 at 12:21:43, dshwang wrote: > > On 2015/06/05 22:05:32, Daniele Castagna wrote: > > > On 2015/06/05 at 19:08:41, dshwang wrote: > > > > I think it's needed. > > > > e.g. if some canvas draws video and then doesn't draw anymore, we want to > > > delete cache earlier than video life cycle. > > > > other example, if user switches tab, we want to delete cache soon. > > > > This caches can consumes few MB of GPU memory. > > > > > > SkImage caches the result internally and I'd assume it purges the cache when > > > it's out of budget. > > > > > > |last_texture_id_| unfortunately takes GPU memory keeping the texture object > > > alive. That will go away as soon as we have Skia support for different > texture > > > targets. At that point we won't need to copy the texture and we'll be able > to > > > get rid of |last_texture_id_|. > > > > > > Would you still suggest to leave that 3 second timer to reset the cache? Why > was > > > that added in the first place? > > > > 3 second timer was introduced to eagerly remove cache to not wait when > > it's out of budget. > > I think it's needed, but I'm not strongly claiming. DaleCurtis, mcasas, what > do you think? > > My rationale for removing the delaytimer is that now we have a plan to get rid > of the cache completely. How will you remove cache for software path? https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:93: mailbox_holder.texture_target, mailbox_holder.mailbox.name); On 2015/06/09 23:21:11, Daniele Castagna wrote: > On 2015/06/08 at 12:21:43, dshwang wrote: > > In this case, this class should not cache the texture. Gpu decoder can reuse > the texture after VideoFrame destruction. Keeping this texture might cause data > race. > > In addition, cache is meaningless in this case, because there is no copy. > > > > The lifetime of the texture id is currently not well defined since SkImage does > not take the ownership of it. > Let's discuss this once we fix that. I guess final solution would be to not cache. why don't you not cache in this case here? I would like to not cache it in TEXTURE_2D case as final solution. https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:318: video_frame->UpdateReleaseSyncPoint(&client); On 2015/06/09 23:21:11, Daniele Castagna wrote: > On 2015/06/08 at 12:21:43, dshwang wrote: > > It's needed only for GL_TEXTURE_2D case. > > That's true, but considering that eventually it will be needed for all the > texture target, I'd prefer not to add specific cases to improve performance for > a code path we're planning to remove. Acknowledged. https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.h (right): https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.h:78: base::TimeDelta last_timestamp_ = media::kNoTimestamp(); On 2015/06/09 23:21:11, Daniele Castagna wrote: > On 2015/06/08 at 12:21:44, dshwang wrote: > > is it allowed to initialize here in chromium coding style? > > It seems so, look for "Non-Static Class Member Initializers" here: > https://chromium-cpp.appspot.com/ Acknowledged. https://codereview.chromium.org/1144323003/diff/370001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.h:90: gpu::gles2::GLES2Interface* last_gl_ = nullptr; On 2015/06/09 23:21:11, Daniele Castagna wrote: > On 2015/06/08 at 12:21:44, dshwang wrote: > > it's very unsafe. it can easily cause sef fault. If you want to keep texture, > keep GrTexture. GrTexture is deleted automatically when gl context is gone. > Agreed. > The idea in this CL is to use SkImage, so I'm not sure using a GrTexture would > be aligned with Skia's goal. > Let me discuss with the Skia team how we can fix this. It'd be nice if > SkImage::NewFromTexture could take ownership of the backend texture and delete > it when done. GrTexture is used in here and there. I think it's fine to use GrTexture. If you uses GrTexture, you don't need to keep GLES2Interface. Keeping GLES2Interface pointer is so dangerous because there is no way to refresh when gpu process down. In addition, when GPU process crash, GrTexture is automatically deleted.
Patchset #13 (id:470001) has been deleted
Patchset #14 (id:510001) has been deleted
Patchset #13 (id:490001) has been deleted
Patchset #13 (id:490001) has been deleted
dongseong.hwang@intel.com can you take a look at this? bsalomon@ implemented SkImage::NewFromAdoptedTexture that makes it easier to deal with GL textures lifetime since SkImages will take ownership of the GL backend textures and will take care of deleting them. The timer that deletes the cache after 3 seconds is also back.
Patchset #14 (id:550001) has been deleted
On 2015/06/25 at 15:48:01, Daniele Castagna wrote: > dongseong.hwang@intel.com can you take a look at this? > > bsalomon@ implemented SkImage::NewFromAdoptedTexture that makes it easier to deal with > GL textures lifetime since SkImages will take ownership of the GL backend textures and will > take care of deleting them. > > The timer that deletes the cache after 3 seconds is also back. Ping.
lgtm, but would like brian's take on the texture/gpu side.
On 2015/07/15 13:32:41, reed1 wrote: > lgtm, but would like brian's take on the texture/gpu side. lgtm too
On 2015/07/15 13:32:41, reed1 wrote: > lgtm, but would like brian's take on the texture/gpu side. lgtm too
Hi, sorry for delaying review. https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:146: mailbox_holder.texture_target, mailbox_holder.mailbox.name); This code bind the texture of the mailbox and then make SkImage keep the texture. After this code, VideoFrame cannot manage the life cycle of this texture. CreateAndConsumeTextureCHROMIUM() is not expensive operation. In this case, I think we should not cache and create |source_texture_| every time. https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:315: DCHECK(gl); if |video_generator_| exists, delete |video_generator_| here. it's because one video can have both software frame and hardware frame interleavingly. https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:323: video_frame->UpdateReleaseSyncPoint(&client); it's duplicated to same code in NewSkImageFromVideoFrameYUVTextures() and SkCanvasVideoRenderer::CopyVideoFrameSingleTextureToGLTexture() in NewSkImageFromVideoFrameNative(). The rule of thumb is that we should update the sync point after completing to use the texture. However in GL_TEXTURE_2D and single plain case, we cannot determine when the texture access is completed because of SkImage cache. https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:326: last_image_ = skia::AdoptRef(SkImage::NewFromGenerator(video_generator_)); now |last_image_| deal with both software SkCanvas and Ganesh SkCanvas. bsalomon, reed, is it safe to use one |last_image_| on both canvases? https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:379: gl->Flush(); Why is this glFlush needed?
https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:326: last_image_ = skia::AdoptRef(SkImage::NewFromGenerator(video_generator_)); On 2015/07/15 15:10:52, dshwang wrote: > now |last_image_| deal with both software SkCanvas and Ganesh SkCanvas. > bsalomon, reed, is it safe to use one |last_image_| on both canvases? It is. If there is a mismatch (e.g. raster-image to gpu-canvas or the reverse) will handle it and cache the conversion.
https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:326: last_image_ = skia::AdoptRef(SkImage::NewFromGenerator(video_generator_)); On 2015/07/15 15:37:32, reed1 wrote: > On 2015/07/15 15:10:52, dshwang wrote: > > now |last_image_| deal with both software SkCanvas and Ganesh SkCanvas. > > bsalomon, reed, is it safe to use one |last_image_| on both canvases? > > It is. If there is a mismatch (e.g. raster-image to gpu-canvas or the reverse) > will handle it and cache the conversion. thx for answering. in this case, gpu or raster image can be drawn on both sw-canvas and gpu-canvas at same time. So gpu or raster image should keep both gpu and raster cache. Is it possible?
https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:326: last_image_ = skia::AdoptRef(SkImage::NewFromGenerator(video_generator_)); On 2015/07/15 at 16:21:10, dshwang wrote: > On 2015/07/15 15:37:32, reed1 wrote: > > On 2015/07/15 15:10:52, dshwang wrote: > > > now |last_image_| deal with both software SkCanvas and Ganesh SkCanvas. > > > bsalomon, reed, is it safe to use one |last_image_| on both canvases? > > > > It is. If there is a mismatch (e.g. raster-image to gpu-canvas or the reverse) > > will handle it and cache the conversion. > > thx for answering. in this case, gpu or raster image can be drawn on both sw-canvas and gpu-canvas at same time. So gpu or raster image should keep both gpu and raster cache. Is it possible? Is that a use case we care about? What's a real example of that happening?
https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:326: last_image_ = skia::AdoptRef(SkImage::NewFromGenerator(video_generator_)); On 2015/07/15 16:27:46, Daniele Castagna wrote: > On 2015/07/15 at 16:21:10, dshwang wrote: > > On 2015/07/15 15:37:32, reed1 wrote: > > > On 2015/07/15 15:10:52, dshwang wrote: > > > > now |last_image_| deal with both software SkCanvas and Ganesh SkCanvas. > > > > bsalomon, reed, is it safe to use one |last_image_| on both canvases? > > > > > > It is. If there is a mismatch (e.g. raster-image to gpu-canvas or the > reverse) > > > will handle it and cache the conversion. > > > > thx for answering. in this case, gpu or raster image can be drawn on both > sw-canvas and gpu-canvas at same time. So gpu or raster image should keep both > gpu and raster cache. Is it possible? > > Is that a use case we care about? What's a real example of that happening? here's real example, ./blink/tools/run_layout_tests.py virtual/gpu/fast/canvas/yuv-video-on-accelerated-canvas.html it's rare in real world, but possible.
On 2015/07/15 16:37:14, dshwang wrote: > https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... > File media/blink/skcanvas_video_renderer.cc (right): > > https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... > media/blink/skcanvas_video_renderer.cc:326: last_image_ = > skia::AdoptRef(SkImage::NewFromGenerator(video_generator_)); > On 2015/07/15 16:27:46, Daniele Castagna wrote: > > On 2015/07/15 at 16:21:10, dshwang wrote: > > > On 2015/07/15 15:37:32, reed1 wrote: > > > > On 2015/07/15 15:10:52, dshwang wrote: > > > > > now |last_image_| deal with both software SkCanvas and Ganesh SkCanvas. > > > > > bsalomon, reed, is it safe to use one |last_image_| on both canvases? > > > > > > > > It is. If there is a mismatch (e.g. raster-image to gpu-canvas or the > > reverse) > > > > will handle it and cache the conversion. > > > > > > thx for answering. in this case, gpu or raster image can be drawn on both > > sw-canvas and gpu-canvas at same time. So gpu or raster image should keep both > > gpu and raster cache. Is it possible? > > > > Is that a use case we care about? What's a real example of that happening? > > here's real example, > ./blink/tools/run_layout_tests.py > virtual/gpu/fast/canvas/yuv-video-on-accelerated-canvas.html > > it's rare in real world, but possible. There are caches in both directions (gpu->raster, raster->gpu) and doing a raster->raster draw won't invalidate the raster->gpu cache entry (and similar for the other direction).
https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:326: last_image_ = skia::AdoptRef(SkImage::NewFromGenerator(video_generator_)); On 2015/07/15 17:01:10, bsalomon wrote: > On 2015/07/15 16:37:14, dshwang wrote: > > > https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... > > File media/blink/skcanvas_video_renderer.cc (right): > > > > > https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... > > media/blink/skcanvas_video_renderer.cc:326: last_image_ = > > skia::AdoptRef(SkImage::NewFromGenerator(video_generator_)); > > On 2015/07/15 16:27:46, Daniele Castagna wrote: > > > On 2015/07/15 at 16:21:10, dshwang wrote: > > > > On 2015/07/15 15:37:32, reed1 wrote: > > > > > On 2015/07/15 15:10:52, dshwang wrote: > > > > > > now |last_image_| deal with both software SkCanvas and Ganesh > SkCanvas. > > > > > > bsalomon, reed, is it safe to use one |last_image_| on both canvases? > > > > > > > > > > It is. If there is a mismatch (e.g. raster-image to gpu-canvas or the > > > reverse) > > > > > will handle it and cache the conversion. > > > > > > > > thx for answering. in this case, gpu or raster image can be drawn on both > > > sw-canvas and gpu-canvas at same time. So gpu or raster image should keep > both > > > gpu and raster cache. Is it possible? > > > > > > Is that a use case we care about? What's a real example of that happening? > > > > here's real example, > > ./blink/tools/run_layout_tests.py > > virtual/gpu/fast/canvas/yuv-video-on-accelerated-canvas.html > > > > it's rare in real world, but possible. > > There are caches in both directions (gpu->raster, raster->gpu) and doing a > raster->raster draw won't invalidate the raster->gpu cache entry (and similar > for the other direction). It sounds good to this change. I agree on |last_image_| handling both sw and gpu canvas.
yuv-video-on-accelerated-canvas.html layout test started failing on linux. Visually inspecting the output it seems the expected image is off by half a pixel. dongseong.hwang@intel.com, do you have more context about yuv-video-on-accelerated-canvas.html? https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:146: mailbox_holder.texture_target, mailbox_holder.mailbox.name); On 2015/07/15 at 15:10:52, dshwang wrote: > This code bind the texture of the mailbox and then make SkImage keep the texture. After this code, VideoFrame cannot manage the life cycle of this texture. > The texture will be deleted after both SkImage and VideoFrame will call glDeleteTextures on their texture names. What problem do you see here? > CreateAndConsumeTextureCHROMIUM() is not expensive operation. In this case, I think we should not cache and create |source_texture_| every time. I agree it's cheap, but the code/logic is simpler without having to avoid caching for this specific case and there isn't any major drawback. https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:315: DCHECK(gl); On 2015/07/15 at 15:10:52, dshwang wrote: > if |video_generator_| exists, delete |video_generator_| here. > it's because one video can have both software frame and hardware frame interleavingly. ResetCache() above takes care of that. https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:323: video_frame->UpdateReleaseSyncPoint(&client); On 2015/07/15 at 15:10:52, dshwang wrote: > it's duplicated to same code in NewSkImageFromVideoFrameYUVTextures() and SkCanvasVideoRenderer::CopyVideoFrameSingleTextureToGLTexture() in NewSkImageFromVideoFrameNative(). > The rule of thumb is that we should update the sync point after completing to use the texture. However in GL_TEXTURE_2D and single plain case, we cannot determine when the texture access is completed because of SkImage cache. Moved UpdateReleaseSyncPoint after we draw. I left the one in CopyVideoFrameSingleTextureToGLTexture since that method is used outside SkCanvasVideoRenderer. https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:379: gl->Flush(); On 2015/07/15 at 15:10:52, dshwang wrote: > Why is this glFlush needed? Gone. I initially put it there to behave in the same way that CopyVideoFrameSingleTextureToGLTexture does.
On 2015/07/17 15:42:41, Daniele Castagna wrote: > yuv-video-on-accelerated-canvas.html layout test started failing on linux. > Visually inspecting the output it seems the expected image is off by half a > pixel. > > mailto:dongseong.hwang@intel.com, do you have more context about > yuv-video-on-accelerated-canvas.html? in my linux machine, it passes; ./blink/tools/run_layout_tests.py virtual/gpu/fast/canvas/yuv-video-on-accelerated-canvas.html This test creates both sw canvas and hw canvas, and then draw one video on both canvases, and then compare the output result with yuv-video-on-accelerated-canvas-expected.html https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:146: mailbox_holder.texture_target, mailbox_holder.mailbox.name); On 2015/07/17 15:42:41, Daniele Castagna wrote: > On 2015/07/15 at 15:10:52, dshwang wrote: > > This code bind the texture of the mailbox and then make SkImage keep the > texture. After this code, VideoFrame cannot manage the life cycle of this > texture. > > > > The texture will be deleted after both SkImage and VideoFrame will call > glDeleteTextures on their texture names. What problem do you see here? let me imagine one worse senario. There are two actors; GVD (gpu video decoder) and painter (this code) GVD: create mailbox painter: refer to this texture GVD: go to recycle process because of the refcount of VideoFrame reach zero. painter: during recycling the texture, it draws the texture on screen. It happens because GVD handle the texture via VideoFrame, but this code make painter escape the texture from VideoFrame mechanism. https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:311: ResetCache(); allocating texture or system memory is expensive. we want to reuse allocated texture or system memory. https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:315: DCHECK(gl); On 2015/07/17 15:42:41, Daniele Castagna wrote: > On 2015/07/15 at 15:10:52, dshwang wrote: > > if |video_generator_| exists, delete |video_generator_| here. > > it's because one video can have both software frame and hardware frame > interleavingly. > > ResetCache() above takes care of that. I have question about ResetCache(). see above comment. https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:323: video_frame->UpdateReleaseSyncPoint(&client); On 2015/07/17 15:42:41, Daniele Castagna wrote: > On 2015/07/15 at 15:10:52, dshwang wrote: > > it's duplicated to same code in NewSkImageFromVideoFrameYUVTextures() and > SkCanvasVideoRenderer::CopyVideoFrameSingleTextureToGLTexture() in > NewSkImageFromVideoFrameNative(). > > The rule of thumb is that we should update the sync point after completing to > use the texture. However in GL_TEXTURE_2D and single plain case, we cannot > determine when the texture access is completed because of SkImage cache. > > Moved UpdateReleaseSyncPoint after we draw. I left the one in > CopyVideoFrameSingleTextureToGLTexture since that method is used outside > SkCanvasVideoRenderer. Acknowledged.
Patchset #16 (id:610001) has been deleted
On 2015/07/20 at 10:47:47, dongseong.hwang wrote: > On 2015/07/17 15:42:41, Daniele Castagna wrote: > > yuv-video-on-accelerated-canvas.html layout test started failing on linux. > > Visually inspecting the output it seems the expected image is off by half a > > pixel. > > > > mailto:dongseong.hwang@intel.com, do you have more context about > > yuv-video-on-accelerated-canvas.html? > > in my linux machine, it passes; ./blink/tools/run_layout_tests.py virtual/gpu/fast/canvas/yuv-video-on-accelerated-canvas.html > > This test creates both sw canvas and hw canvas, and then draw one video on both canvases, and then compare the output result with yuv-video-on-accelerated-canvas-expected.html > We investigated a little on this. Converting YUV to RGBA using SkCanvasVideoRenderer::ConvertVideoFrameToRGBPixels doesn't produce the same output as using GrYUVtoRGBEffect to do the conversion. One difference being that GrYUVtoRGBEffect uses bilerp filtering while the ConvertVideoFrameToRGBPixels doesn't. That means that creating a SkImage, drawing to a software canvas and then to an hardware one produces a slightly different result than drawing directly to an hardware canvas. This happens because Skia caches the software converted RGBA and uses that in the first case, while it uses GrYUVtoRGBEffect in the second case. We discussed this with Dale and decided to invalidate the cache if the canvas changes from hardware to software (or the other way). While there might be a performance regression (yuv-video-on-accelerated-canvas.html test takes .12 more to run), this use case is unlikely to happen in real life. https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:146: mailbox_holder.texture_target, mailbox_holder.mailbox.name); On 2015/07/20 at 10:47:47, dshwang wrote: > On 2015/07/17 15:42:41, Daniele Castagna wrote: > > On 2015/07/15 at 15:10:52, dshwang wrote: > > > This code bind the texture of the mailbox and then make SkImage keep the > > texture. After this code, VideoFrame cannot manage the life cycle of this > > texture. > > > > > > > The texture will be deleted after both SkImage and VideoFrame will call > > glDeleteTextures on their texture names. What problem do you see here? > > let me imagine one worse senario. > There are two actors; GVD (gpu video decoder) and painter (this code) > GVD: create mailbox > painter: refer to this texture > GVD: go to recycle process because of the refcount of VideoFrame reach zero. GVD waits for release_sync_point_ in VideoFrame before recycling the texture. Look at GpuVideoDecoder::ReleaseMailbox. > painter: during recycling the texture, it draws the texture on screen. > > It happens because GVD handle the texture via VideoFrame, but this code make painter escape the texture from VideoFrame mechanism. https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:311: ResetCache(); On 2015/07/20 at 10:47:47, dshwang wrote: > allocating texture or system memory is expensive. we want to reuse allocated texture or system memory. Expensive compared to what? We used to reallocate VideoImageGenerator before this patch too, so that is not getting worse. SkImages are immutables, so if we want to switch to that interface we have to reallocate a new SkImage for every new VideoFrame. Internally SkImage tries to avoid reallocating resources.
Sorry for delaying review. On 2015/07/30 19:55:00, Daniele Castagna wrote: > On 2015/07/20 at 10:47:47, dongseong.hwang wrote: > > On 2015/07/17 15:42:41, Daniele Castagna wrote: > > > yuv-video-on-accelerated-canvas.html layout test started failing on linux. > > > Visually inspecting the output it seems the expected image is off by half a > > > pixel. > > > > > > mailto:dongseong.hwang@intel.com, do you have more context about > > > yuv-video-on-accelerated-canvas.html? > > > > in my linux machine, it passes; ./blink/tools/run_layout_tests.py > virtual/gpu/fast/canvas/yuv-video-on-accelerated-canvas.html > > > > This test creates both sw canvas and hw canvas, and then draw one video on > both canvases, and then compare the output result with > yuv-video-on-accelerated-canvas-expected.html > > > > We investigated a little on this. > Converting YUV to RGBA using SkCanvasVideoRenderer::ConvertVideoFrameToRGBPixels > doesn't produce the same output as using GrYUVtoRGBEffect to do the conversion. > One difference being that GrYUVtoRGBEffect uses bilerp filtering while the > ConvertVideoFrameToRGBPixels doesn't. > > That means that creating a SkImage, drawing to a software canvas and then to an > hardware one produces a slightly different result than drawing directly to an > hardware canvas. > This happens because Skia caches the software converted RGBA and uses that in > the first case, while it uses GrYUVtoRGBEffect in the second case. > > We discussed this with Dale and decided to invalidate the cache if the canvas > changes from hardware to software (or the other way). While there might be a > performance regression (yuv-video-on-accelerated-canvas.html test takes .12 > more to run), this use case is unlikely to happen in real life. I think we don't have to invalidate the cache. The reason makes sense. In this case, we should change yuv-video-on-accelerated-canvas-expected.html. Remove yuv-video-on-accelerated-canvas-expected.html and add "crbug.com/449197 virtual/gpu/fast/canvas/yuv-video-on-accelerated-canvas.html [ NeedsRebaseline ]" in https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... The bot will generate new result png files. https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:146: mailbox_holder.texture_target, mailbox_holder.mailbox.name); On 2015/07/30 19:55:00, Daniele Castagna wrote: > On 2015/07/20 at 10:47:47, dshwang wrote: > > On 2015/07/17 15:42:41, Daniele Castagna wrote: > > > On 2015/07/15 at 15:10:52, dshwang wrote: > > > > This code bind the texture of the mailbox and then make SkImage keep the > > > texture. After this code, VideoFrame cannot manage the life cycle of this > > > texture. > > > > > > > > > > The texture will be deleted after both SkImage and VideoFrame will call > > > glDeleteTextures on their texture names. What problem do you see here? > > > > let me imagine one worse senario. > > There are two actors; GVD (gpu video decoder) and painter (this code) > > GVD: create mailbox > > painter: refer to this texture > > GVD: go to recycle process because of the refcount of VideoFrame reach zero. > > GVD waits for release_sync_point_ in VideoFrame before recycling the texture. > Look at GpuVideoDecoder::ReleaseMailbox. That's problem. While GVD waits for |release_sync_point_|, |last_image_| holds the texture. GVD cannot know the time in which |last_image_| releases the texture. Before this change, there is not any code to hold VideoFrame or the mailbox texture for undetermined period. I think the most practical solution is to not cache the |source_texture| in this case, although code would be bloating. > > painter: during recycling the texture, it draws the texture on screen. > > > > It happens because GVD handle the texture via VideoFrame, but this code make > painter escape the texture from VideoFrame mechanism. https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:311: ResetCache(); On 2015/07/30 19:54:59, Daniele Castagna wrote: > On 2015/07/20 at 10:47:47, dshwang wrote: > > allocating texture or system memory is expensive. we want to reuse allocated > texture or system memory. > > Expensive compared to what? > We used to reallocate VideoImageGenerator before this patch too, so that is not > getting worse. > > SkImages are immutables, so if we want to switch to that interface we have to > reallocate a new SkImage for every new VideoFrame. Internally SkImage tries to > avoid reallocating resources. no, previously reuse cached SkBitmap without reallocation. I'm not sure this perf regression is acceptable. In sw path, tcmalloc might reallocate same sized memory without overhead. However, in hw path, it's not acceptable to delete texture and then create new texture. https://codereview.chromium.org/1144323003/diff/630001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (left): https://codereview.chromium.org/1144323003/diff/630001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:366: accelerated_last_image_.reset(); It caused perf regression. when it used SkBitmap, the cached texture was reused. In hw path, we should reuse GrTexture and make SkImage not manage lifecycle of the GrTexture. https://codereview.chromium.org/1144323003/diff/630001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:420: last_frame_.height() != video_frame->visible_rect().height()) { in sw case, reused |last_frame_| if size is same. now we reallocate every frame. https://codereview.chromium.org/1144323003/diff/630001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/630001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:159: mailbox_holder.texture_target, mailbox_holder.mailbox.name); As mentioned above, we should cache in order to reuse |source_texture| if mailbox_holder.texture_target != GL_TEXTURE_2D. however, we should not cache in order to avoid outlive of |source_texture| if mailbox_holder.texture_target == GL_TEXTURE_2D
https://codereview.chromium.org/1144323003/diff/630001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (left): https://codereview.chromium.org/1144323003/diff/630001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:366: accelerated_last_image_.reset(); On 2015/08/03 17:51:52, dshwang wrote: > It caused perf regression. when it used SkBitmap, the cached texture was reused. > In hw path, we should reuse GrTexture and make SkImage not manage lifecycle of > the GrTexture. SkImage will recycle GrTextures internally. Skia is trying to remove GrTexture from the public API. We'd like Chrome code to stop using it. The refScratchTexture API is now deprecated.
Patchset #18 (id:670001) has been deleted
Patchset #17 (id:650001) has been deleted
On 2015/08/03 at 17:51:53, dongseong.hwang wrote: > Sorry for delaying review. > > On 2015/07/30 19:55:00, Daniele Castagna wrote: > > On 2015/07/20 at 10:47:47, dongseong.hwang wrote: > > > On 2015/07/17 15:42:41, Daniele Castagna wrote: > > > > yuv-video-on-accelerated-canvas.html layout test started failing on linux. > > > > Visually inspecting the output it seems the expected image is off by half a > > > > pixel. > > > > > > > > mailto:dongseong.hwang@intel.com, do you have more context about > > > > yuv-video-on-accelerated-canvas.html? > > > > > > in my linux machine, it passes; ./blink/tools/run_layout_tests.py > > virtual/gpu/fast/canvas/yuv-video-on-accelerated-canvas.html > > > > > > This test creates both sw canvas and hw canvas, and then draw one video on > > both canvases, and then compare the output result with > > yuv-video-on-accelerated-canvas-expected.html > > > > > > > We investigated a little on this. > > Converting YUV to RGBA using SkCanvasVideoRenderer::ConvertVideoFrameToRGBPixels > > doesn't produce the same output as using GrYUVtoRGBEffect to do the conversion. > > One difference being that GrYUVtoRGBEffect uses bilerp filtering while the > > ConvertVideoFrameToRGBPixels doesn't. > > > > That means that creating a SkImage, drawing to a software canvas and then to an > > hardware one produces a slightly different result than drawing directly to an > > hardware canvas. > > This happens because Skia caches the software converted RGBA and uses that in > > the first case, while it uses GrYUVtoRGBEffect in the second case. > > > > We discussed this with Dale and decided to invalidate the cache if the canvas > > changes from hardware to software (or the other way). While there might be a > > performance regression (yuv-video-on-accelerated-canvas.html test takes .12 > > more to run), this use case is unlikely to happen in real life. > > I think we don't have to invalidate the cache. The reason makes sense. In this case, we should change yuv-video-on-accelerated-canvas-expected.html. > Remove yuv-video-on-accelerated-canvas-expected.html and add "crbug.com/449197 virtual/gpu/fast/canvas/yuv-video-on-accelerated-canvas.html [ NeedsRebaseline ]" in > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > The bot will generate new result png files. > Cool. The cache invalidation has been removed, PTAL at https://codereview.chromium.org/1266113003 https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:146: mailbox_holder.texture_target, mailbox_holder.mailbox.name); On 2015/08/03 at 17:51:52, dshwang wrote: > On 2015/07/30 19:55:00, Daniele Castagna wrote: > > On 2015/07/20 at 10:47:47, dshwang wrote: > > > On 2015/07/17 15:42:41, Daniele Castagna wrote: > > > > On 2015/07/15 at 15:10:52, dshwang wrote: > > > > > This code bind the texture of the mailbox and then make SkImage keep the > > > > texture. After this code, VideoFrame cannot manage the life cycle of this > > > > texture. > > > > > > > > > > > > > The texture will be deleted after both SkImage and VideoFrame will call > > > > glDeleteTextures on their texture names. What problem do you see here? > > > > > > let me imagine one worse senario. > > > There are two actors; GVD (gpu video decoder) and painter (this code) > > > GVD: create mailbox > > > painter: refer to this texture > > > GVD: go to recycle process because of the refcount of VideoFrame reach zero. > > > > GVD waits for release_sync_point_ in VideoFrame before recycling the texture. > > Look at GpuVideoDecoder::ReleaseMailbox. > > That's problem. While GVD waits for |release_sync_point_|, |last_image_| holds the texture. GVD cannot know the time in which |last_image_| releases the texture. > Before this change, there is not any code to hold VideoFrame or the mailbox texture for undetermined period. This wouldn't be a problem since last_image_ is not used unless we receive that same VideoFrame. Said so, I changed the code to drop the cache as you suggested since you don't like the idea of keeping around a texture even if we won't use it. > > I think the most practical solution is to not cache the |source_texture| in this case, although code would be bloating. > > > > painter: during recycling the texture, it draws the texture on screen. > > > > > > It happens because GVD handle the texture via VideoFrame, but this code make > > painter escape the texture from VideoFrame mechanism. https://codereview.chromium.org/1144323003/diff/570001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:311: ResetCache(); On 2015/08/03 at 17:51:52, dshwang wrote: > On 2015/07/30 19:54:59, Daniele Castagna wrote: > > On 2015/07/20 at 10:47:47, dshwang wrote: > > > allocating texture or system memory is expensive. we want to reuse allocated > > texture or system memory. > > > > Expensive compared to what? > > We used to reallocate VideoImageGenerator before this patch too, so that is not > > getting worse. > > > > SkImages are immutables, so if we want to switch to that interface we have to > > reallocate a new SkImage for every new VideoFrame. Internally SkImage tries to > > avoid reallocating resources. > > no, previously reuse cached SkBitmap without reallocation. I'm not sure this perf regression is acceptable. > > In sw path, tcmalloc might reallocate same sized memory without overhead. However, in hw path, it's not acceptable to delete texture and then create new texture. Please refer to Brian's answer. https://codereview.chromium.org/1144323003/diff/630001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (left): https://codereview.chromium.org/1144323003/diff/630001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:420: last_frame_.height() != video_frame->visible_rect().height()) { On 2015/08/03 at 17:51:52, dshwang wrote: > in sw case, reused |last_frame_| if size is same. now we reallocate every frame. We can't do that with SkImage API. Skia might reuse an already allocated Skbitmap internally. https://codereview.chromium.org/1144323003/diff/630001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/630001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:159: mailbox_holder.texture_target, mailbox_holder.mailbox.name); On 2015/08/03 at 17:51:53, dshwang wrote: > As mentioned above, we should cache in order to reuse |source_texture| if mailbox_holder.texture_target != GL_TEXTURE_2D. however, we should not cache in order to avoid outlive of |source_texture| if mailbox_holder.texture_target == GL_TEXTURE_2D OK, now the cache gets invalidated after we draw to the canvas.
https://codereview.chromium.org/1144323003/diff/630001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (left): https://codereview.chromium.org/1144323003/diff/630001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:366: accelerated_last_image_.reset(); On 2015/08/03 18:12:44, bsalomon wrote: > On 2015/08/03 17:51:52, dshwang wrote: > > It caused perf regression. when it used SkBitmap, the cached texture was > reused. > > In hw path, we should reuse GrTexture and make SkImage not manage lifecycle of > > the GrTexture. > > SkImage will recycle GrTextures internally. In this case, skia cannot recycle the texture. NewSkImageFromVideoFrameNative() creates new texture every frame via glGenTextures. Texture creation code is out of control from skia. > Skia is trying to remove GrTexture > from the public API. We'd like Chrome code to stop using it. The > refScratchTexture API is now deprecated. Is wrapBackendTexture API also deprecated? In this case, GrTexture is ideal because GrTexture can internally delete its own texture on gpu crash. In my opinion, GrTexture is hard to abuse and chromium needs it. https://codereview.chromium.org/1144323003/diff/690001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/690001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:331: ResetCache(); need to reuse |source_texture| in "video_frame->mailbox_holder(0).texture_target != GL_TEXTURE_2D" case. Note that NewSkImageFromVideoFrameNative() create new texture via glGenTexture and glCopyTextureCHROMIUM allocates new texture if the texture is not allocated. https://codereview.chromium.org/1144323003/diff/690001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:399: video_frame->UpdateReleaseSyncPoint(&client); It should be after ResetCache()
https://codereview.chromium.org/1144323003/diff/630001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (left): https://codereview.chromium.org/1144323003/diff/630001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:366: accelerated_last_image_.reset(); On 2015/08/04 09:51:36, dshwang wrote: > On 2015/08/03 18:12:44, bsalomon wrote: > > On 2015/08/03 17:51:52, dshwang wrote: > > > It caused perf regression. when it used SkBitmap, the cached texture was > > reused. > > > In hw path, we should reuse GrTexture and make SkImage not manage lifecycle > of > > > the GrTexture. > > > > SkImage will recycle GrTextures internally. > > In this case, skia cannot recycle the texture. NewSkImageFromVideoFrameNative() > creates new texture every frame via glGenTextures. Texture creation code is out > of control from skia. I see, but wasn't it doing that both before and after this CL? The 3 planes get merged into an RGBA texture that is created by the SkImage constructor which internally uses the cache. > > > Skia is trying to remove GrTexture > > from the public API. We'd like Chrome code to stop using it. The > > refScratchTexture API is now deprecated. > > Is wrapBackendTexture API also deprecated? In this case, GrTexture is ideal > because GrTexture can internally delete its own texture on gpu crash. > In my opinion, GrTexture is hard to abuse and chromium needs it. The preferred APIs are now SkImage:: NewFromTexture, NewFromTextureCopy, and NewFromAdoptedTexture. The first is a way of importing an external texture as an SkImage (it is built on top of wrapBackendTexture). The second is used when Chromium can't promise to not alter or delete the texture before Skia consumes it. The constructor copies the external texture into a scratch texture owned by Skia. The third transfers ownership of a texture to Skia, meaning that Skia will delete the texture after the SkImage is destroyed (and any uses of it are flushed to GrContext).
https://codereview.chromium.org/1144323003/diff/690001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/690001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:399: video_frame->UpdateReleaseSyncPoint(&client); On 2015/08/04 at 09:51:36, dshwang wrote: > It should be after ResetCache() Done.
I think there isn't no bug about correctness. However, there are rooms to optimize performance more. Let's revisit it later. Thank you for patient review. lgtm.
lgtm assuming skia folk and dshwang@ are happy. https://codereview.chromium.org/1144323003/diff/710001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/710001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:324: if (last_image_ && video_frame->timestamp() == last_timestamp_) { Seems pretty likely to be wrong, but the old code does it and hasn't seen any issues I guess. You could check the frame ptr or mark it with some VideoFrameMetadata.
Patchset #19 (id:730001) has been deleted
Patchset #18 (id:730001) has been deleted
Patchset #19 (id:750001) has been deleted
Patchset #19 (id:770001) has been deleted
The last patch avoids setting to null the videoframe associated to the VideoImageGenerator since it was causing a few tests to fail. Waiting for crrev.com/1273163005 and then I'll try to land this. https://codereview.chromium.org/1144323003/diff/710001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/710001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:324: if (last_image_ && video_frame->timestamp() == last_timestamp_) { On 2015/08/07 at 17:10:39, DaleCurtis wrote: > Seems pretty likely to be wrong, but the old code does it and hasn't seen any issues I guess. You could check the frame ptr or mark it with some VideoFrameMetadata. I misunderstood this. Initially I thought there was one SkCanvasVideoRenderer per canvas, but there is one per WebMediaPlayer. As long as there is only one video per WebMediaPlayer this check should work. Comment removed.
https://codereview.chromium.org/1144323003/diff/790001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/790001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:273: scoped_refptr<VideoFrame> frame_; DaleCurtis, is it ok that VideoFrame live long? I think it's fine, but fischman did be reluctant about it 1 years ago.
https://codereview.chromium.org/1144323003/diff/790001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/790001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:273: scoped_refptr<VideoFrame> frame_; On 2015/08/12 at 11:22:51, dshwang wrote: > DaleCurtis, is it ok that VideoFrame live long? I think it's fine, but fischman did be reluctant about it 1 years ago. What were fischman's concerns?
dongseong.hwang@intel.com changed reviewers: + danakj@chromium.org, scherkus@chromium.org
https://codereview.chromium.org/1144323003/diff/790001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/790001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:273: scoped_refptr<VideoFrame> frame_; On 2015/08/12 18:17:54, Daniele Castagna wrote: > On 2015/08/12 at 11:22:51, dshwang wrote: > > DaleCurtis, is it ok that VideoFrame live long? I think it's fine, but > fischman did be reluctant about it 1 years ago. > > What were fischman's concerns? Sorry, I mis-remember why the patch set 18 was introduced in https://codereview.chromium.org/531353002/ Other clients of VideoFrame (e.g. Compositor, WebGL) returns VideoFrame immediately, but it's the first time for client to keep VideoFrame in unexpected period. So I was little nervous. However, I couldn't imagine it causes any issues. So lgtm. danakj, scherkus, do you think so?
danakj, can you LGTM for cc/test/* owner approval? https://codereview.chromium.org/1144323003/diff/790001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/790001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:273: scoped_refptr<VideoFrame> frame_; On 2015/08/13 at 09:58:47, dshwang wrote: > On 2015/08/12 18:17:54, Daniele Castagna wrote: > > On 2015/08/12 at 11:22:51, dshwang wrote: > > > DaleCurtis, is it ok that VideoFrame live long? I think it's fine, but > > fischman did be reluctant about it 1 years ago. > > > > What were fischman's concerns? > > Sorry, I mis-remember why the patch set 18 was introduced in https://codereview.chromium.org/531353002/ > > Other clients of VideoFrame (e.g. Compositor, WebGL) returns VideoFrame immediately, but it's the first time for client to keep VideoFrame in unexpected period. So I was little nervous. > However, I couldn't imagine it causes any issues. So lgtm. > Cool, all the tests are passing now. I'm going to try to land this, I'll make sure to be around if something goes wrong. > danakj, scherkus, do you think so?
Can you explain the changes in cc/? They aren't explained in the patch description at all, that I could see. https://codereview.chromium.org/1144323003/diff/790001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/790001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:68: const scoped_refptr<VideoFrame>& video_frame, btw if you're not passing a scoped_refptr to take a ref on it (i don't see you taking a ref here), then pass a VideoFrame* instead. https://codereview.chromium.org/1144323003/diff/790001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:132: const scoped_refptr<VideoFrame>& video_frame) { ditto
https://codereview.chromium.org/1144323003/diff/790001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/790001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:273: scoped_refptr<VideoFrame> frame_; On 2015/08/13 15:44:25, Daniele Castagna wrote: > On 2015/08/13 at 09:58:47, dshwang wrote: > > On 2015/08/12 18:17:54, Daniele Castagna wrote: > > > On 2015/08/12 at 11:22:51, dshwang wrote: > > > > DaleCurtis, is it ok that VideoFrame live long? I think it's fine, but > > > fischman did be reluctant about it 1 years ago. > > > > > > What were fischman's concerns? > > > > Sorry, I mis-remember why the patch set 18 was introduced in > https://codereview.chromium.org/531353002/ > > > > Other clients of VideoFrame (e.g. Compositor, WebGL) returns VideoFrame > immediately, but it's the first time for client to keep VideoFrame in unexpected > period. So I was little nervous. > > However, I couldn't imagine it causes any issues. So lgtm. > > > Cool, all the tests are passing now. I'm going to try to land this, I'll make > sure to be around if something goes wrong. > > > danakj, scherkus, do you think so? Hmm, you only seem to be holding the video frame in the software case which is okay since you have a cleanup timer. Likely the original concern was probably that the GPUVideoDecoder only has a fixed set of picture buffers and holding the frame will prevent it from releasing one of those. It doesn't look like you're holding the video frame when textures are present though, you're making a copy. If that's the case this is fine.
On Thu, Aug 13, 2015 at 9:43 AM, <dalecurtis@chromium.org> wrote: > > > https://codereview.chromium.org/1144323003/diff/790001/media/blink/skcanvas_v... > File media/blink/skcanvas_video_renderer.cc (right): > > > https://codereview.chromium.org/1144323003/diff/790001/media/blink/skcanvas_v... > media/blink/skcanvas_video_renderer.cc:273: scoped_refptr<VideoFrame> > frame_; > On 2015/08/13 15:44:25, Daniele Castagna wrote: > >> On 2015/08/13 at 09:58:47, dshwang wrote: >> > On 2015/08/12 18:17:54, Daniele Castagna wrote: >> > > On 2015/08/12 at 11:22:51, dshwang wrote: >> > > > DaleCurtis, is it ok that VideoFrame live long? I think it's >> > fine, but > >> > > fischman did be reluctant about it 1 years ago. >> > > >> > > What were fischman's concerns? >> > >> > Sorry, I mis-remember why the patch set 18 was introduced in >> https://codereview.chromium.org/531353002/ >> > >> > Other clients of VideoFrame (e.g. Compositor, WebGL) returns >> > VideoFrame > >> immediately, but it's the first time for client to keep VideoFrame in >> > unexpected > >> period. So I was little nervous. >> > However, I couldn't imagine it causes any issues. So lgtm. >> > >> Cool, all the tests are passing now. I'm going to try to land this, >> > I'll make > >> sure to be around if something goes wrong. >> > > > danakj, scherkus, do you think so? >> > > Hmm, you only seem to be holding the video frame in the software case > which is okay since you have a cleanup timer. > > Likely the original concern was probably that the GPUVideoDecoder only > has a fixed set of picture buffers and holding the frame will prevent it > from releasing one of those. It doesn't look like you're holding the > video frame when textures are present though, you're making a copy. If > that's the case this is fine. > If that's the reasoning it sounds complex, can we document this in the code? > > https://codereview.chromium.org/1144323003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Yeah, that's a good idea. How about around l.321? Lets add a DCHECK(!HasTextures()) to the VIG constructor too.
Patchset #20 (id:810001) has been deleted
danakj, there is now more info about the change in cc at the end of the CL description. A comment and a DCHECK in VIG have also been added to clarify video_frame lifetime. https://codereview.chromium.org/1144323003/diff/790001/media/blink/skcanvas_v... File media/blink/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1144323003/diff/790001/media/blink/skcanvas_v... media/blink/skcanvas_video_renderer.cc:68: const scoped_refptr<VideoFrame>& video_frame, On 2015/08/13 at 16:31:20, danakj wrote: > btw if you're not passing a scoped_refptr to take a ref on it (i don't see you taking a ref here), then pass a VideoFrame* instead. I think that's the convention used in media, dalecurtis suggested to use a const scoped_refptr reference in different occasions. Take a look at media/cast/test/sender.cc LogRawEvents for another example.
On 2015/08/13 17:44:59, Daniele Castagna wrote: > danakj, there is now more info about the change in cc at the end of the CL > description. > > A comment and a DCHECK in VIG have also been added to clarify video_frame > lifetime. > > https://codereview.chromium.org/1144323003/diff/790001/media/blink/skcanvas_v... > File media/blink/skcanvas_video_renderer.cc (right): > > https://codereview.chromium.org/1144323003/diff/790001/media/blink/skcanvas_v... > media/blink/skcanvas_video_renderer.cc:68: const scoped_refptr<VideoFrame>& > video_frame, > On 2015/08/13 at 16:31:20, danakj wrote: > > btw if you're not passing a scoped_refptr to take a ref on it (i don't see you > taking a ref here), then pass a VideoFrame* instead. > > I think that's the convention used in media, dalecurtis suggested to use a const > scoped_refptr reference in different occasions. > > Take a look at media/cast/test/sender.cc LogRawEvents for another example. OK well I'll just disagree then, from my own corner of the woods. Passing const scoped_refptr<T>& is not clear about your intentions at all, and means code might start taking references and it wouldn't be obvious. Passing a T* makes your intent clear so the reader can look at the call and reason about the reference ownership behaviour. And when passing a ref to a method, I recommend passing it by value now, and use .Pass() to assign it to the new local storage. This behaviour is at least as or more efficient than using const scoped_refptr<T>& which can add a temporary reference sometimes that it doesn't need to, so you can get a ref you wouldn't with T*, and you can get 2 increments and a decrement when you just wanted one increment.
Thanks, cc/ LGTM
On 2015/08/13 17:51:38, danakj wrote: > On 2015/08/13 17:44:59, Daniele Castagna wrote: > > danakj, there is now more info about the change in cc at the end of the CL > > description. > > > > A comment and a DCHECK in VIG have also been added to clarify video_frame > > lifetime. > > > > > https://codereview.chromium.org/1144323003/diff/790001/media/blink/skcanvas_v... > > File media/blink/skcanvas_video_renderer.cc (right): > > > > > https://codereview.chromium.org/1144323003/diff/790001/media/blink/skcanvas_v... > > media/blink/skcanvas_video_renderer.cc:68: const scoped_refptr<VideoFrame>& > > video_frame, > > On 2015/08/13 at 16:31:20, danakj wrote: > > > btw if you're not passing a scoped_refptr to take a ref on it (i don't see > you > > taking a ref here), then pass a VideoFrame* instead. > > > > I think that's the convention used in media, dalecurtis suggested to use a > const > > scoped_refptr reference in different occasions. > > > > Take a look at media/cast/test/sender.cc LogRawEvents for another example. > > OK well I'll just disagree then, from my own corner of the woods. > > Passing const scoped_refptr<T>& is not clear about your intentions at all, and > means code might start taking references and it wouldn't be obvious. Passing a > T* makes your intent clear so the reader can look at the call and reason about > the reference ownership behaviour. And when passing a ref to a method, I > recommend passing it by value now, and use .Pass() to assign it to the new local > storage. > > This behaviour is at least as or more efficient than using const > scoped_refptr<T>& which can add a temporary reference sometimes that it doesn't > need to, so you can get a ref you wouldn't with T*, and you can get 2 increments > and a decrement when you just wanted one increment. I agree with danakj@, my previous statement was more about passing the scoped_refptr to callers by copy. I'd even go further and say pass const VideoFrame& or const VideoFrame* where possible.
On 2015/08/13 at 18:13:46, dalecurtis wrote: > On 2015/08/13 17:51:38, danakj wrote: > > On 2015/08/13 17:44:59, Daniele Castagna wrote: > > > danakj, there is now more info about the change in cc at the end of the CL > > > description. > > > > > > A comment and a DCHECK in VIG have also been added to clarify video_frame > > > lifetime. > > > > > > > > https://codereview.chromium.org/1144323003/diff/790001/media/blink/skcanvas_v... > > > File media/blink/skcanvas_video_renderer.cc (right): > > > > > > > > https://codereview.chromium.org/1144323003/diff/790001/media/blink/skcanvas_v... > > > media/blink/skcanvas_video_renderer.cc:68: const scoped_refptr<VideoFrame>& > > > video_frame, > > > On 2015/08/13 at 16:31:20, danakj wrote: > > > > btw if you're not passing a scoped_refptr to take a ref on it (i don't see > > you > > > taking a ref here), then pass a VideoFrame* instead. > > > > > > I think that's the convention used in media, dalecurtis suggested to use a > > const > > > scoped_refptr reference in different occasions. > > > > > > Take a look at media/cast/test/sender.cc LogRawEvents for another example. > > > > OK well I'll just disagree then, from my own corner of the woods. > > > > Passing const scoped_refptr<T>& is not clear about your intentions at all, and > > means code might start taking references and it wouldn't be obvious. Passing a > > T* makes your intent clear so the reader can look at the call and reason about > > the reference ownership behaviour. And when passing a ref to a method, I > > recommend passing it by value now, and use .Pass() to assign it to the new local > > storage. > > > > This behaviour is at least as or more efficient than using const > > scoped_refptr<T>& which can add a temporary reference sometimes that it doesn't > > need to, so you can get a ref you wouldn't with T*, and you can get 2 increments > > and a decrement when you just wanted one increment. > > I agree with danakj@, my previous statement was more about passing the scoped_refptr to callers by copy. I'd even go further and say pass const VideoFrame& or const VideoFrame* where possible. Cool. Done.
The CQ bit was checked by dcastagna@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com, reed@google.com, dalecurtis@chromium.org, dongseong.hwang@intel.com, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/1144323003/#ps850001 (title: "s/scoped_refptr<VideoFrame>&/VideoFrame*/ where possible.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1144323003/850001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1144323003/850001
Message was sent while issue was closed.
Committed patchset #21 (id:850001)
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/5bf77af1588dbd967ad909eebec2e380dcb669a6 Cr-Commit-Position: refs/heads/master@{#343272} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
