Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
445013002:
media: Optimize HW Video to 2D Canvas copy. (Closed)
|
Created:
6 years, 4 months ago by dshwang Modified:
5 years, 11 months ago Reviewers:
jamesr, Lei Zhang, Justin Novosad, rileya (GONE FROM CHROMIUM), Aaron Boodman, no sievers, perkj_chrome, Charlie Reis, xhwang, Ben Goodger (Google), scherkus (not reviewing) CC:
chromium-reviews, yusukes+watch_chromium.org, posciak+watch_chromium.org, sievers+watch_chromium.org, yukishiino+watch_chromium.org, miu+watch_chromium.org, jam, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, timurrrr+watch_chromium.org, kalyank, bruening+watch_chromium.org, penghuang+watch_chromium.org, feature-media-reviews_chromium.org, piman+watch_chromium.org, danakj+watch_chromium.org, wjia+watch_chromium.org, avayvod+watch_chromium.org, mcasas+watch_chromium.org, cc-bugs_chromium.org, James Su, glider+watch_chromium.org, piman, Charlie Reis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia: Optimize HW Video to 2D Canvas copy.
Currently, when we draws GPU decoded Video on accelerated 2D Canvas, chromium
reads back pixel from GPU and then uploads the pixel to GPU to make a SkBitmap.
It's so inefficient for both speed and battery. On the other hand, only Android
copies GPU-GPU in this case, but Android doesn't have cache mechanism which
SkCanvasVideoRenderer provides.
This CL makes all platforms copy gpu-gpu with cache mechanism. Cache mechanism
is useful when 2d canvas draws a video frame many times.
e.g. http://craftymind.com/factory/html5video/CanvasVideo.html
In addition, fix white video background on Android when not loaded. Other platforms
draw black background thanks to SkCanvasVideoRenderer::Paint().
In detail of the changes;
1. Implement gpu-gpu copy in SkCanvasVideoRenderer::Paint() like previous
WebMediaPlayerAndroid::paint().
2. Move duplicated GPU code on WebMediaPlayerImpl and WebMediaPlayerAndroid to
SkCanvasVideoRenderer.
Perf data on i5 IvyBridge
blink_perf.all:Canvas_draw-video-to-hw-accelerated-canvas-2d
15.8x speed up: 116.27 runs/s -> 1847.23 runs/s
NOTE: measure after disabling cache in SkCanvasVideoRenderer
BUG=401058, 263667
Committed: https://crrev.com/0c4e9d8781aea6e52fdb4a7aee978817910c67ea
Cr-Commit-Position: refs/heads/master@{#310577}
Patch Set 1 #
Total comments: 2
Patch Set 2 : build fix and pass presubmit #Patch Set 3 : Fix cc unittests #
Total comments: 3
Patch Set 4 : Don't move SkCanvasVideoRenderer #
Total comments: 5
Patch Set 5 : rename ContextProvider to Context3DProvider #
Total comments: 1
Patch Set 6 : Rebase to ToT #
Total comments: 7
Patch Set 7 : Replace magic number, fix rotation for hw path #
Total comments: 2
Patch Set 8 : Use scratch texture #
Total comments: 4
Patch Set 9 : Rebase to ToT. Address nits #
Total comments: 14
Patch Set 10 : Resolve comments, rebase to ToT #
Total comments: 44
Patch Set 11 : Address nits #
Total comments: 1
Patch Set 12 : DCHECK context_provider and rebase to ToT #
Total comments: 3
Patch Set 13 : SkCanvasVideoRenderer use WebGraphicsContext3D #
Total comments: 1
Patch Set 14 : SkCanvasVideoRenderer use GLES2Interface, not WebGraphicsContext3D #Patch Set 15 : fix window component build #
Total comments: 1
Patch Set 16 : rename from Context3DProvider to Context3D #
Total comments: 2
Patch Set 17 : Remove unrelated change in render_frame_impl.cc #Patch Set 18 : rollback android code #
Total comments: 6
Patch Set 19 : add comment about kRenderTarget #Patch Set 20 : rebase to ToT #
Total comments: 3
Messages
Total messages: 104 (20 generated)
thestig@: Please review changes in tools/valgrind/drmemory/suppressions_full.txt sievers@: Please review changes in content/browser/renderer_host/render_widget_host_view_browsertest.cc scherkus@, acolwell@: Please review changes in media/ and content/renderer/media danakj@chromium.org: Please review changes in cc/ https://codereview.chromium.org/445013002/diff/1/cc/DEPS File cc/DEPS (right): https://codereview.chromium.org/445013002/diff/1/cc/DEPS#newcode14 cc/DEPS:14: "+third_party/libyuv/include/libyuv.h", danakj@, It's not necessary. See below comment. https://codereview.chromium.org/445013002/diff/1/cc/resources/media/skcanvas_... File cc/resources/media/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/445013002/diff/1/cc/resources/media/skcanvas_... cc/resources/media/skcanvas_video_renderer.cc:28: #define LIBYUV_I422_TO_ARGB libyuv::I422ToABGR Now cc depends on thirdparty/libyuv directly. However, we can avoid it, if we make wrapper of those functions in yuv_convert.h. For example, Define media::ConvertI422ToABGR() in media/base/yuv_convert.h and then here just calls media::ConvertI422ToABGR(). media::ConvertI422ToABGR() would be void ConvertI422ToABGR(...) { libyuv::I420ToABGR(...); } WDYT? danakj@, scherkus@, acolwell@
tools/valgrind lgtm
https://codereview.chromium.org/445013002/diff/40001/cc/resources/media/skcan... File cc/resources/media/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/445013002/diff/40001/cc/resources/media/skcan... cc/resources/media/skcanvas_video_renderer.cc:224: gpu::gles2::GLES2Interface* gl = context_provider->ContextGL(); Why does this need to move to cc to use a ContextProvider?
https://codereview.chromium.org/445013002/diff/40001/cc/resources/media/skcan... File cc/resources/media/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/445013002/diff/40001/cc/resources/media/skcan... cc/resources/media/skcanvas_video_renderer.cc:224: gpu::gles2::GLES2Interface* gl = context_provider->ContextGL(); Yes, because of ContextProvider. However, your question is very good. after re-thinking, if client passes GLES2Interface and GrContext instead of ContextProvider, this moving is not necessary. Do you think passing two arguments is better?
aelias@: Please review changes in content/browser/renderer_host/render_widget_host_view_browsertest.cc scherkus@, acolwell@: Please review changes in media/ and content/renderer/media danakj@: Please review changes in cc/ After re-thinking of danakj@'s comment, I realized it's not necessary to move SkCanvasVideoRenderer. Now patch set become more compact.
https://codereview.chromium.org/445013002/diff/40001/cc/resources/media/skcan... File cc/resources/media/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/445013002/diff/40001/cc/resources/media/skcan... cc/resources/media/skcanvas_video_renderer.cc:224: gpu::gles2::GLES2Interface* gl = context_provider->ContextGL(); On 2014/08/07 05:50:07, dshwang wrote: > Yes, because of ContextProvider. > However, your question is very good. after re-thinking, if client passes > GLES2Interface and GrContext instead of ContextProvider, this moving is not > necessary. > Do you think passing two arguments is better? Yeah, probably. It seems weird to move media code into cc/. Maybe we should consider moving the ContextProvider interface definition out of cc so we don't have this issue also. +piman
https://codereview.chromium.org/445013002/diff/60001/cc/resources/video_resou... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/445013002/diff/60001/cc/resources/video_resou... cc/resources/video_resource_updater.cc:256: 0); This should be NULL right? It's a pointer? https://codereview.chromium.org/445013002/diff/60001/media/filters/skcanvas_v... File media/filters/skcanvas_video_renderer.h (right): https://codereview.chromium.org/445013002/diff/60001/media/filters/skcanvas_v... media/filters/skcanvas_video_renderer.h:33: struct ContextProvider { Please use a different name for this, it's a bit confusing. Add a suffix or something, just anything not exactly this. :)
I measured perf data and describe above description. 15.8x speed up on Ubuntu IvyBridge. https://codereview.chromium.org/445013002/diff/60001/cc/resources/video_resou... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/445013002/diff/60001/cc/resources/video_resou... cc/resources/video_resource_updater.cc:256: 0); Done. I was confused with Blink coding style. https://codereview.chromium.org/445013002/diff/60001/media/filters/skcanvas_v... File media/filters/skcanvas_video_renderer.h (right): https://codereview.chromium.org/445013002/diff/60001/media/filters/skcanvas_v... media/filters/skcanvas_video_renderer.h:33: struct ContextProvider { Done. Now it's Context3DProvider. btw I intended to preserve our tradition. :) https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... https://codereview.chromium.org/445013002/diff/80001/media/filters/skcanvas_v... File media/filters/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/445013002/diff/80001/media/filters/skcanvas_v... media/filters/skcanvas_video_renderer.cc:302: // timestamp. All platforms except for Android speed up ~15x To speed up Android, Android video decoder must set timestamp to a video frame but it's tricky because Android uses stream texture.
https://codereview.chromium.org/445013002/diff/60001/media/filters/skcanvas_v... File media/filters/skcanvas_video_renderer.h (right): https://codereview.chromium.org/445013002/diff/60001/media/filters/skcanvas_v... media/filters/skcanvas_video_renderer.h:33: struct ContextProvider { On 2014/08/07 15:10:43, dshwang wrote: > Done. Now it's Context3DProvider. > btw I intended to preserve our tradition. :) > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... Oh I had no idea that existed too.. heh.
On Thu, Aug 7, 2014 at 7:06 AM, <danakj@chromium.org> wrote: > > https://codereview.chromium.org/445013002/diff/40001/cc/ > resources/media/skcanvas_video_renderer.cc > File cc/resources/media/skcanvas_video_renderer.cc (right): > > https://codereview.chromium.org/445013002/diff/40001/cc/ > resources/media/skcanvas_video_renderer.cc#newcode224 > cc/resources/media/skcanvas_video_renderer.cc:224: > gpu::gles2::GLES2Interface* gl = context_provider->ContextGL(); > On 2014/08/07 05:50:07, dshwang wrote: > >> Yes, because of ContextProvider. >> However, your question is very good. after re-thinking, if client >> > passes > >> GLES2Interface and GrContext instead of ContextProvider, this moving >> > is not > >> necessary. >> Do you think passing two arguments is better? >> > > Yeah, probably. It seems weird to move media code into cc/. > > Maybe we should consider moving the ContextProvider interface definition > out of cc so we don't have this issue also. +piman > Maybe it can go in gpu/, provided the skia dependency doesn't cause loops. > > https://codereview.chromium.org/445013002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I rebase this CL after crrev.com/441303002/ rileya@, scherkus@, could you review media/ and content/renderer/media ?
Hi rileya@, scherkus@, could you review media/ and content/renderer/media ?
I wholeheartedly support the idea behind this patch (the various video->canvas/WebGL paths are just awful), but I have a good number of questions/concerns. Someone with a better big-picture view of the chromium/Blink graphics situation should really have a look at this (junov@ maybe?). https://codereview.chromium.org/445013002/diff/120001/media/filters/skcanvas_... File media/filters/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/445013002/diff/120001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:323: if (!accelerated && (last_frame_.isNull() || It doesn't look like we address video rotation in the accelerated case, does this mean we lose video orientation support for HW decoded frames? I realize that we don't properly do this for WebGL, but in the accelerated canvas case I think we can actually apply the rotation without much fuss... https://codereview.chromium.org/445013002/diff/120001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:368: static const base::TimeDelta buffer_time = base::TimeDelta::FromSeconds(3); This seems extremely arbitrary. Can you explain the reasoning? At the very least this might be better as a constant on the SkCanvasRenderer class itself, along with a comment on why we're using the magic number of 3 seconds. https://codereview.chromium.org/445013002/diff/120001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:382: void SkCanvasVideoRenderer::CopyVideoFrameToTexture( I'm not sure how I feel about putting all of this GL stuff tacked onto SkCanvasVideoRenderer, this seems like something Blink should be able to do. That said, this may be a necessary evil, I've recently been looking at the SW video->WebGL case and my current solution also requires some similar ugliness... (https://codereview.chromium.org/513593003/ for reference) https://codereview.chromium.org/445013002/diff/120001/media/filters/skcanvas_... File media/filters/skcanvas_video_renderer.h (right): https://codereview.chromium.org/445013002/diff/120001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.h:71: SkBitmap accelerated_last_frame_; If there are multiple separate hardware accelerated canvases (presumably on separate Ganesh/GL contexts) painting frames from SkCanvasVideoRenderer at once, couldn't there be issues with sharing this texture? I'm not terribly familiar with the Blink/Ganesh integration, so I may be off base here, but this seems fishy...
Patchset #7 (id:140001) has been deleted
On 2014/08/27 20:30:50, rileya wrote: > I wholeheartedly support the idea behind this patch (the various > video->canvas/WebGL paths are just awful), but I have a good number of > questions/concerns. Thank you for reviewing CL wholeheartedly :) > Someone with a better big-picture view of the chromium/Blink graphics situation > should really have a look at this (junov@ maybe?). The main purpose of this CL is to accelerate drawImage(video) and drawImage(canvas) of canvas 2d api on chromebook. junov@, could you review this CL? > https://codereview.chromium.org/445013002/diff/120001/media/filters/skcanvas_... > File media/filters/skcanvas_video_renderer.cc (right): > > https://codereview.chromium.org/445013002/diff/120001/media/filters/skcanvas_... > media/filters/skcanvas_video_renderer.cc:323: if (!accelerated && > (last_frame_.isNull() || > It doesn't look like we address video rotation in the accelerated case, does > this mean we lose video orientation support for HW decoded frames? Oops, it's mistake when rebasing this CL. Now I apply rotation to HW path also. > I realize that we don't properly do this for WebGL, but in the accelerated > canvas case I think we can actually apply the rotation without much fuss... texImage2D(hardware canvas) is still broken in terms of rotation. I think previous solution is not optimal; last_frame_ = SkBitmapOperations::Rotate(last_frame_, SkBitmapOperations::ROTATION_270_CW), because 1. it requires additional copy 2. it's not easy to apply same solution to texImage2D(hardware canvas) IMO we should inform rotation to the compositor and compositor should handle it without copy. The solution fixes texImage2D(hardware canvas) automatically. danakj@, WDYT? > https://codereview.chromium.org/445013002/diff/120001/media/filters/skcanvas_... > media/filters/skcanvas_video_renderer.cc:368: static const base::TimeDelta > buffer_time = base::TimeDelta::FromSeconds(3); > This seems extremely arbitrary. Can you explain the reasoning? > > At the very least this might be better as a constant on the SkCanvasRenderer > class itself, along with a comment on why we're using the magic number of 3 > seconds. Yes, I define kTemporaryResourceDeletionDelay and explain by comment. // This class keeps two temporary resources; software bitmap, hardware bitmap. // If both bitmap are created and then only software bitmap is updated every // frame, hardware bitmap outlives until the media player dies. So we delete // a temporary resource if it is not used for 3 sec. const int kTemporaryResourceDeletionDelay = 3; // Seconds; > https://codereview.chromium.org/445013002/diff/120001/media/filters/skcanvas_... > media/filters/skcanvas_video_renderer.cc:382: void > SkCanvasVideoRenderer::CopyVideoFrameToTexture( > I'm not sure how I feel about putting all of this GL stuff tacked onto > SkCanvasVideoRenderer, this seems like something Blink should be able to do. > That said, this may be a necessary evil, I've recently been looking at the SW > video->WebGL case and my current solution also requires some similar ugliness... > (https://codereview.chromium.org/513593003/ for reference) I agree that CopyVideoFrameToTexture() doesn't fit naturally to SkCanvasVideoRenderer, but I didn't find better place. 1. WebMediaPlayerImpl and WebMediaPlayerAndroid uses it in the similar purpose of SkCanvasVideoRenderer::Paint. 2. Hardware path of SkCanvasVideoRenderer::Paint() needs CopyVideoFrameToTexture() also. > https://codereview.chromium.org/445013002/diff/120001/media/filters/skcanvas_... > File media/filters/skcanvas_video_renderer.h (right): > > https://codereview.chromium.org/445013002/diff/120001/media/filters/skcanvas_... > media/filters/skcanvas_video_renderer.h:71: SkBitmap accelerated_last_frame_; > If there are multiple separate hardware accelerated canvases (presumably on > separate Ganesh/GL contexts) painting frames from SkCanvasVideoRenderer at once, > couldn't there be issues with sharing this texture? > > I'm not terribly familiar with the Blink/Ganesh integration, so I may be off > base here, but this seems fishy... That's good question. It's safe because GrContext is from RenderThreadImpl::current()->SharedMainThreadContextProvider().
https://codereview.chromium.org/445013002/diff/120001/media/filters/skcanvas_... File media/filters/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/445013002/diff/120001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:207: texture = skia::AdoptRef(gr->createUncachedTexture(desc, 0, 0)); Using a scratch texture would probably be better for performance https://codereview.chromium.org/445013002/diff/160001/media/filters/skcanvas_... File media/filters/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/445013002/diff/160001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:420: GL_TEXTURE_2D, source_texture, texture, level, internal_format, type); If this changes the state of the 'texture', you probably need to invalidate the state cached in skia because this texture belongs to a GrTexture.
Thank you for review https://codereview.chromium.org/445013002/diff/120001/media/filters/skcanvas_... File media/filters/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/445013002/diff/120001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:207: texture = skia::AdoptRef(gr->createUncachedTexture(desc, 0, 0)); On 2014/08/28 17:23:18, junov wrote: > Using a scratch texture would probably be better for performance Yep, I'll update. https://codereview.chromium.org/445013002/diff/160001/media/filters/skcanvas_... File media/filters/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/445013002/diff/160001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:420: GL_TEXTURE_2D, source_texture, texture, level, internal_format, type); On 2014/08/28 17:23:19, junov wrote: > If this changes the state of the 'texture', you probably need to invalidate the > state cached in skia because this texture belongs to a GrTexture. Ah, I didn't think about it. however fortunately, CopyTextureCHROMIUM doesn't change any GL states. and GL_UNPACK_PREMULTIPLY_ALPHA_CHROMIUM and GL_UNPACK_FLIP_Y_CHROMIUM are not exposed to skia. I'll add a comment which makes others be careful of this code.
rileya@, could you review again? https://codereview.chromium.org/445013002/diff/120001/media/filters/skcanvas_... File media/filters/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/445013002/diff/120001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:207: texture = skia::AdoptRef(gr->createUncachedTexture(desc, 0, 0)); On 2014/08/28 17:23:18, junov wrote: > Using a scratch texture would probably be better for performance Done.
lgtm with a couple style nits, but this touches lots of Blink/graphics stuff that I'm not too familiar with, so others should give it a look. Also, I'm not an OWNER, so scherkus@ will need to sign off on media/. > texImage2D(hardware canvas) is still broken in terms of rotation. > I think previous solution is not optimal; last_frame_ = > SkBitmapOperations::Rotate(last_frame_, SkBitmapOperations::ROTATION_270_CW), > because > 1. it requires additional copy > 2. it's not easy to apply same solution to texImage2D(hardware canvas) Yeah, this is slow. What we should do is rotate/translate on the canvas itself and paint the rotated image directly, rather than doing a copy. Something for a future CL though, not really relevant to this. https://codereview.chromium.org/445013002/diff/180001/content/renderer/media/... File content/renderer/media/webmediaplayer_impl.cc (right): https://codereview.chromium.org/445013002/diff/180001/content/renderer/media/... content/renderer/media/webmediaplayer_impl.cc:570: if (!video_frame || Multi-line conditional needs {}'s. https://codereview.chromium.org/445013002/diff/180001/media/filters/skcanvas_... File media/filters/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/445013002/diff/180001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:384: if (last_timestamp > last_frame_timestamp_ + buffer_time && Multi-line conditionals need {}'s. https://codereview.chromium.org/445013002/diff/180001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:387: if (last_timestamp > accelerated_last_frame_timestamp_ + buffer_time && ditto https://codereview.chromium.org/445013002/diff/180001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:414: // Application itself needs to take care of setting the right flip_y In media/ we enclose variable names in ||'s, so this should be |flip_y|.
dongseong.hwang@intel.com changed reviewers: - aelias@chromium.org, danakj@chromium.org
dongseong.hwang@intel.com changed reviewers: + jamesr@chromium.org
On 2014/09/02 21:21:43, rileya wrote: > lgtm with a couple style nits, but this touches lots of Blink/graphics stuff > that I'm not too familiar with, so others should give it a look. > > Also, I'm not an OWNER, so scherkus@ will need to sign off on media/. Thank you for reviewing this quite complicated CL. All nits done. scherkus@, could you review media/ and content/renderer/media ? jamesr@, could you review content/renderer/render_frame_impl.cc and mojo/services/html_viewer/webmediaplayer_factory.cc ? > > texImage2D(hardware canvas) is still broken in terms of rotation. > > I think previous solution is not optimal; last_frame_ = > > SkBitmapOperations::Rotate(last_frame_, SkBitmapOperations::ROTATION_270_CW), > > because > > 1. it requires additional copy > > 2. it's not easy to apply same solution to texImage2D(hardware canvas) > > Yeah, this is slow. What we should do is rotate/translate on the canvas itself > and paint the rotated image directly, rather than doing a copy. Something for a > future CL though, not really relevant to this. It's resolved in https://codereview.chromium.org/619343003/ as rileya@ explained. Rebasing to ToT requires some big changes because webmediaplayer_impl.cc is moved from content/renderer to media/blink in the previous CL. webmediaplayer_impl.cc cann't use RenderThreadImpl::current()->SharedMainThreadContextProvider(), so I added the callback member to query the context provider in webmediaplayer_params.h Clients creating a WebMediaPlayerImpl instance must pass base::Callback<cc::ContextProvider*()> callback. media/blink/webmediaplayer_impl.cc needs to depend on webkit/common/gpu/webgraphicscontext3d_impl.h so I add webkit/common/gpu in media/blink/DEPS. Is it acceptable? If it's not acceptable, I'll add another callback object in webmediaplayer_params.h or move back webgraphicscontext3d_impl.h into content/renderer as the Owner suggests. https://codereview.chromium.org/445013002/diff/200001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.h (left): https://codereview.chromium.org/445013002/diff/200001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.h:124: // https://code.google.com/p/skia/issues/detail?id=1189 Remove the stale comment. Currently, blink uses WebMediaPlayerAndroid::Paint in the same way to WebMediaPlayer(Impl|MS)::Paint https://codereview.chromium.org/445013002/diff/200001/media/DEPS File media/DEPS (right): https://codereview.chromium.org/445013002/diff/200001/media/DEPS#newcode5 media/DEPS:5: "+skia/ext", To use skia::RefPtr. If I use SkAutoTUnref to avoid adding skia/ext dependency, presubmit fails with message "use skia::RefPtr". https://codereview.chromium.org/445013002/diff/200001/media/blink/DEPS File media/blink/DEPS (right): https://codereview.chromium.org/445013002/diff/200001/media/blink/DEPS#newcode11 media/blink/DEPS:11: "+webkit/common/gpu", It's the addition which I ask. https://codereview.chromium.org/445013002/diff/200001/mojo/services/html_view... File mojo/services/html_viewer/webmediaplayer_factory.cc (right): https://codereview.chromium.org/445013002/diff/200001/mojo/services/html_view... mojo/services/html_viewer/webmediaplayer_factory.cc:56: scoped_refptr<media::GpuVideoAcceleratorFactories>(), mojo didn't care of gpu decoding because of null GpuVideoAcceleratorFactories object, so I pass null ContextProviderCB callback object.
SkCanvasVideoRenderer is really a class that knows how to deal with software-backed frames. I don't think mixing in all the NATIVE_TEXTURE-handling code into is worth the additional complexity for what used to be a pretty simple class. Also, it's fairly safe to assume that the format of a video frame won't change back and forth from native to non-native during the lifetime of a WebMediaPlayer. It seems like what we really need is to refactor all this code into a few helper classes: - SoftwareVideoFrameToSkBitmap (the existing conversion code in SkCanvasVideoRenderer) - NativeTextureToSkBitmap (your new conversion code in this CL) - SkBitmapToSkCanvas (the existing rendering code in SkCanvasVideoRenderer that handles rotation, scaling, etc...) Any thoughts? Would you feel comfortable refactoring prior to landing this CL or would it be easier for you to do so after landing? https://codereview.chromium.org/445013002/diff/200001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/445013002/diff/200001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:518: shared_main_thread_context_provider_cb_.Run(); is the context guaranteed to not change throughout the lifetime of the process? in other words, can we call this once instead of every paint()?
On 2014/10/13 17:48:41, scherkus wrote: Thank you for quick and great comments. > SkCanvasVideoRenderer is really a class that knows how to deal with > software-backed frames. I don't think mixing in all the NATIVE_TEXTURE-handling > code into is worth the additional complexity for what used to be a pretty simple > class. Although SkCanvasVideoRenderer looks like the class for only software-backed frames, currently SkCanvasVideoRenderer::Paint slightly handles all combinations; sw-video-to-sw-canvas, hw-video-to-sw-canvas, sw-video-to-hw-canvas, hw-video-to-hw-canvas 1. sw-video-to-sw-canvas : create RGB bitmap, and draw it on the canvas 2. hw-video-to-sw-canvas : create RGB bitmap via readback pixel, and draw it on canvas; readback is done via VideoFrame::ReadPixelsFromNativeTexture(SkBitmap*) method. 3. sw-video-to-hw-canvas : create YUV texture or RGB bitmap, and draw it on canvas 4. hw-video-to-hw-canvas : create RGB bitmap via readback pixel, and draw it on canvas; it causes gpu-cpu copy and then cpu-gpu copy. This CL's goal is the optimization of #4; create RGB texture, and draw it on canvas. Interestingly, WebMediaPlayerAndroid::Paint already optimized #4 path in its own code. So this CL merges SkCanvasVideoRenderer::Paint with WebMediaPlayerAndroid::Paint into SkCanvasVideoRenderer::Paint After that, SkCanvasVideoRenderer includes all the NATIVE_TEXTURE-handling code, so extract the code into SkCanvasVideoRenderer::CopyVideoFrameToTexture() to reuse it. It's story how this CL made SkCanvasVideoRenderer bigger and bigger. > Also, it's fairly safe to assume that the format of a video frame won't change > back and forth from native to non-native during the lifetime of a > WebMediaPlayer. VideoFrame's format is immutable as you mentioned, but required canvas can be changed. Think about sw-canvas and hw-canvas requests to draw video on its canvas. Even if enabling accelerated 2d canvas, small canvas (<127x127) is software canvas to save gpu resources. This means current SkCanvasVideoRenderer::Paint implementation has a serious bug, and this CL fixes it; cache SkBitmap respectively for sw-video-to-sw-canvas and sw-video-to-hw-canvas For example, hw-canvas draw video and then sw-canvas draw video 1. hw-canvas draw video; SkCanvasVideoRenderer creates SkBitmap as YUV texture, and caches it. 2. sw-canvas draw video: SkCanvasVideoRenderer try to draw the cached SkBimap which previous GrContext created on sw-canvas. Is it safe? It may cause crash. > It seems like what we really need is to refactor all this code into a few helper > classes: > - SoftwareVideoFrameToSkBitmap (the existing conversion code in > SkCanvasVideoRenderer) > - NativeTextureToSkBitmap (your new conversion code in this CL) > - SkBitmapToSkCanvas (the existing rendering code in SkCanvasVideoRenderer > that handles rotation, scaling, etc...) > > Any thoughts? Would you feel comfortable refactoring prior to landing this CL or > would it be easier for you to do so after landing? If media/ doesn't want to include any gpu code, I can create new class to handle hw-video-to-sw-canvas and hw-video-to-hw-canvas and it will belong to content/renderer/media However, it requires all WebMediaPlayer implementations, webmediaplayer_(android|impl|ms), checks whether SkCanvas is accelerated or not, and VideoFrame is accelerated or not. I think it's better to encapsulate the login in SkCanvasVideoRenderer. So I suggests how about moving SkCanvasVideoRenderer to content/renderer/media? I can refactor this prior to this CL including your suggesting helper classes and bug fix as I mentioned above. If we decide to move SkCanvasVideoRenderer, WebMediaPlayerImpl also needs to be moved to content/renderer/media because it uses SkCanvasVideoRenderer. > https://codereview.chromium.org/445013002/diff/200001/media/blink/webmediapla... > File media/blink/webmediaplayer_impl.cc (right): > > https://codereview.chromium.org/445013002/diff/200001/media/blink/webmediapla... > media/blink/webmediaplayer_impl.cc:518: > shared_main_thread_context_provider_cb_.Run(); > is the context guaranteed to not change throughout the lifetime of the process? > > in other words, can we call this once instead of every paint()? That's good suggestion, but we can not guarantee the context is immutable. i.e. gpu process crash and then create new shared context.
On 2014/10/13 18:42:10, dshwang wrote: > On 2014/10/13 17:48:41, scherkus wrote: > > Thank you for quick and great comments. > > > SkCanvasVideoRenderer is really a class that knows how to deal with > > software-backed frames. I don't think mixing in all the > NATIVE_TEXTURE-handling > > code into is worth the additional complexity for what used to be a pretty > simple > > class. > > Although SkCanvasVideoRenderer looks like the class for only software-backed > frames, currently SkCanvasVideoRenderer::Paint slightly handles all > combinations; sw-video-to-sw-canvas, hw-video-to-sw-canvas, > sw-video-to-hw-canvas, hw-video-to-hw-canvas > 1. sw-video-to-sw-canvas : create RGB bitmap, and draw it on the canvas > 2. hw-video-to-sw-canvas : create RGB bitmap via readback pixel, and draw it on > canvas; readback is done via VideoFrame::ReadPixelsFromNativeTexture(SkBitmap*) > method. > 3. sw-video-to-hw-canvas : create YUV texture or RGB bitmap, and draw it on > canvas > 4. hw-video-to-hw-canvas : create RGB bitmap via readback pixel, and draw it on > canvas; it causes gpu-cpu copy and then cpu-gpu copy. > > This CL's goal is the optimization of #4; create RGB texture, and draw it on > canvas. Interestingly, WebMediaPlayerAndroid::Paint already optimized #4 path in > its own code. > So this CL merges SkCanvasVideoRenderer::Paint with WebMediaPlayerAndroid::Paint > into SkCanvasVideoRenderer::Paint > After that, SkCanvasVideoRenderer includes all the NATIVE_TEXTURE-handling code, > so extract the code into SkCanvasVideoRenderer::CopyVideoFrameToTexture() to > reuse it. > It's story how this CL made SkCanvasVideoRenderer bigger and bigger. Thanks for the explanation. I'm OK having SkCanvasVideoRenderer get bigger but until you gave this explanation the various code paths and combinations wasn't really clear when reading the code. I made a few suggestions on method naming that could help clear this up. Some additional documentation inside Paint() would be helpful. > > Also, it's fairly safe to assume that the format of a video frame won't change > > back and forth from native to non-native during the lifetime of a > > WebMediaPlayer. > > VideoFrame's format is immutable as you mentioned, but required canvas can be > changed. > Think about sw-canvas and hw-canvas requests to draw video on its canvas. Even > if enabling accelerated 2d canvas, small canvas (<127x127) is software canvas to > save gpu resources. > This means current SkCanvasVideoRenderer::Paint implementation has a serious > bug, and this CL fixes it; cache SkBitmap respectively for sw-video-to-sw-canvas > and sw-video-to-hw-canvas > For example, hw-canvas draw video and then sw-canvas draw video > 1. hw-canvas draw video; SkCanvasVideoRenderer creates SkBitmap as YUV texture, > and caches it. > 2. sw-canvas draw video: SkCanvasVideoRenderer try to draw the cached SkBimap > which previous GrContext created on sw-canvas. Is it safe? It may cause crash. > > > It seems like what we really need is to refactor all this code into a few > helper > > classes: > > - SoftwareVideoFrameToSkBitmap (the existing conversion code in > > SkCanvasVideoRenderer) > > - NativeTextureToSkBitmap (your new conversion code in this CL) > > - SkBitmapToSkCanvas (the existing rendering code in SkCanvasVideoRenderer > > that handles rotation, scaling, etc...) > > > > Any thoughts? Would you feel comfortable refactoring prior to landing this CL > or > > would it be easier for you to do so after landing? > > If media/ doesn't want to include any gpu code, I can create new class to handle > hw-video-to-sw-canvas and hw-video-to-hw-canvas and it will belong to > content/renderer/media > However, it requires all WebMediaPlayer implementations, > webmediaplayer_(android|impl|ms), checks whether SkCanvas is accelerated or not, > and VideoFrame is accelerated or not. > I think it's better to encapsulate the login in SkCanvasVideoRenderer. > So I suggests how about moving SkCanvasVideoRenderer to content/renderer/media? > I can refactor this prior to this CL including your suggesting helper classes > and bug fix as I mentioned above. > If we decide to move SkCanvasVideoRenderer, WebMediaPlayerImpl also needs to be > moved to content/renderer/media because it uses SkCanvasVideoRenderer. It's OK to have media/ depend on gpu/ -- we've actually been moving more code out of content/renderer/media/ to media/ so we're definitely not moving it back :) > > > https://codereview.chromium.org/445013002/diff/200001/media/blink/webmediapla... > > File media/blink/webmediaplayer_impl.cc (right): > > > > > https://codereview.chromium.org/445013002/diff/200001/media/blink/webmediapla... > > media/blink/webmediaplayer_impl.cc:518: > > shared_main_thread_context_provider_cb_.Run(); > > is the context guaranteed to not change throughout the lifetime of the > process? > > > > in other words, can we call this once instead of every paint()? > > That's good suggestion, but we can not guarantee the context is immutable. i.e. > gpu process crash and then create new shared context.
https://codereview.chromium.org/445013002/diff/200001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/445013002/diff/200001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:598: skcanvas_video_renderer_.CopyVideoFrameToTexture(gl, use SkCanvasVideoRenderer:: instead of the object -- there's no actual state being modified on the object https://codereview.chromium.org/445013002/diff/200001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/445013002/diff/200001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:600: skcanvas_video_renderer_.CopyVideoFrameToTexture(gl, use SkCanvasVideoRenderer:: instead of the object -- there's no actual state being modified on the object https://codereview.chromium.org/445013002/diff/200001/media/filters/skcanvas_... File media/filters/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/445013002/diff/200001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:43: const int kTemporaryResourceDeletionDelay = 3; // Seconds; note this is in media time, which has little relation to actual wall clock time can you provide some more details on why we need this? for example, what would happen if we didn't have this code? https://codereview.chromium.org/445013002/diff/200001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:222: bool EnsureTextureBackedSkBitmap(GrContext* gr, I know you lifted this code from WebMediaPlayerAndroid, but can we rename this, add docs, and clean it up a bit? It's a small style thing, but I don't really like these "ensure" style methods as it gets unclear as to what they exactly are doing and what returning "false" means (it looks like it's for saying that it failed to allocate) for example, I'd find it more clear to split this into two functions: bool IsSkBitmapProperlySizedTexture(const SkBitmap* bitmap, const gfx::Size& size) { // existing initial checks } bool AllocateSkBitmapTexture(GrContext* gr, SkBitmap* bitmap, const gfx::Size& size) { // rest of allocation code } It'd help clean up the comments/code around line 266 in CovertVideoFrameToTexture() https://codereview.chromium.org/445013002/diff/200001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:255: bool ConvertVideoFrameToTexture( considering all the various combinations of sw vs. hw, let's try to have these method names be as exact as possible I'd also recommend changing the name Convert to Copy considering it is actually a copy e.g., CopyVideoFrameTextureToSkBitmapTexture(...) https://codereview.chromium.org/445013002/diff/200001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:272: unsigned textureId = textureId -> texture_id https://codereview.chromium.org/445013002/diff/200001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:409: SkCanvasVideoRenderer::Context3DProvider* context_provider) { we can drop SkCanvasVideoRenderer::, right? https://codereview.chromium.org/445013002/diff/200001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:551: void SkCanvasVideoRenderer::CopyVideoFrameToTexture( ditto for precise naming this is much more like CopyVideoFrameTextureToGlTexture() https://codereview.chromium.org/445013002/diff/200001/media/filters/skcanvas_... File media/filters/skcanvas_video_renderer.h (right): https://codereview.chromium.org/445013002/diff/200001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.h:37: struct Context3DProvider { to be clear, we need this because we don't depend on cc/, right?
I extracted separate logic crrev.com/445013002 from this CL. Now I rebase this to ToT after the CL is landed. And resolve all comments. On 2014/10/15 17:10:24, scherkus wrote: > https://codereview.chromium.org/445013002/diff/200001/content/renderer/media/... > File content/renderer/media/android/webmediaplayer_android.cc (right): > > https://codereview.chromium.org/445013002/diff/200001/content/renderer/media/... > content/renderer/media/android/webmediaplayer_android.cc:598: > skcanvas_video_renderer_.CopyVideoFrameToTexture(gl, > use SkCanvasVideoRenderer:: instead of the object -- there's no actual state > being modified on the object That's right. Done. > https://codereview.chromium.org/445013002/diff/200001/media/blink/webmediapla... > File media/blink/webmediaplayer_impl.cc (right): > > https://codereview.chromium.org/445013002/diff/200001/media/blink/webmediapla... > media/blink/webmediaplayer_impl.cc:600: > skcanvas_video_renderer_.CopyVideoFrameToTexture(gl, > use SkCanvasVideoRenderer:: instead of the object -- there's no actual state > being modified on the object Done. > https://codereview.chromium.org/445013002/diff/200001/media/filters/skcanvas_... > media/filters/skcanvas_video_renderer.cc:222: bool > EnsureTextureBackedSkBitmap(GrContext* gr, > I know you lifted this code from WebMediaPlayerAndroid, but can we rename this, > add docs, and clean it up a bit? It's a small style thing, but I don't really > like these "ensure" style methods as it gets unclear as to what they exactly are > doing and what returning "false" means (it looks like it's for saying that it > failed to allocate) > > > for example, I'd find it more clear to split this into two functions: > > bool IsSkBitmapProperlySizedTexture(const SkBitmap* bitmap, const gfx::Size& > size) { > // existing initial checks > } > > bool AllocateSkBitmapTexture(GrContext* gr, SkBitmap* bitmap, const gfx::Size& > size) { > // rest of allocation code > } > > > It'd help clean up the comments/code around line 266 in > CovertVideoFrameToTexture() Thank you! I use this naming in the new patch set. > https://codereview.chromium.org/445013002/diff/200001/media/filters/skcanvas_... > media/filters/skcanvas_video_renderer.cc:255: bool ConvertVideoFrameToTexture( > considering all the various combinations of sw vs. hw, let's try to have these > method names be as exact as possible > > I'd also recommend changing the name Convert to Copy considering it is actually > a copy > > e.g., CopyVideoFrameTextureToSkBitmapTexture(...) Yep, Done. > https://codereview.chromium.org/445013002/diff/200001/media/filters/skcanvas_... > media/filters/skcanvas_video_renderer.cc:272: unsigned textureId = > textureId -> texture_id Done. > https://codereview.chromium.org/445013002/diff/200001/media/filters/skcanvas_... > media/filters/skcanvas_video_renderer.cc:551: void > SkCanvasVideoRenderer::CopyVideoFrameToTexture( > ditto for precise naming > > this is much more like CopyVideoFrameTextureToGlTexture() Done as CopyVideoFrameTextureToGLTexture because other code use the term "GLTexture" in cc > https://codereview.chromium.org/445013002/diff/200001/media/filters/skcanvas_... > File media/filters/skcanvas_video_renderer.h (right): > > https://codereview.chromium.org/445013002/diff/200001/media/filters/skcanvas_... > media/filters/skcanvas_video_renderer.h:37: struct Context3DProvider { > to be clear, we need this because we don't depend on cc/, right? I introduce media/filters/context_3d_provider.h to remove the dependency on cc::ContextProvier, because of cc depends on media. media should not depend on cc. content layer feeds media::Context3DProvider and SkCanvasVideoRenderer uses media::Context3DProvider, instead of cc::ContextProvier. > https://codereview.chromium.org/445013002/diff/200001/media/filters/skcanvas_... > media/filters/skcanvas_video_renderer.cc:409: > SkCanvasVideoRenderer::Context3DProvider* context_provider) { > we can drop SkCanvasVideoRenderer::, right? now Context3DProvider is media::Context3DProvider, so I drop SkCanvasVideoRenderer::. FYI, SkCanvasVideoRenderer:: cannot be dropped in previous patch set.
https://codereview.chromium.org/445013002/diff/520001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/445013002/diff/520001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:544: (video_frame.get() && nit: is the .get() needed? https://codereview.chromium.org/445013002/diff/520001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:599: video_frame.get(), nit: is .get() needed? https://codereview.chromium.org/445013002/diff/520001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/445013002/diff/520001/content/renderer/render... content/renderer/render_frame_impl.cc:480: DCHECK(render_thread); nit: I'm not sure how useful these DCHECKs are in practice as current() will return nullptr if you don't access this on the right thread (it relies on TLS) we also dereference on line 483 -- so we're essentially checking for the same condition multiple times in a variety of different ways https://codereview.chromium.org/445013002/diff/520001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/445013002/diff/520001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:524: (video_frame.get() && nit: is .get() needed? https://codereview.chromium.org/445013002/diff/520001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:533: skcanvas_video_renderer_.Paint(video_frame.get(), nit: is .get() needed? https://codereview.chromium.org/445013002/diff/520001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:599: if (!video_frame.get() || nit: is .get() needed? https://codereview.chromium.org/445013002/diff/520001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:610: video_frame.get(), nit: is .get() needed? https://codereview.chromium.org/445013002/diff/520001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/445013002/diff/520001/media/blink/webmediapla... media/blink/webmediaplayer_impl.h:275: Context3DProviderCB shared_main_thread_context_3d_provider_cb_; ditto for naming: drop the the "shared_main_thread_" part https://codereview.chromium.org/445013002/diff/520001/media/blink/webmediapla... File media/blink/webmediaplayer_params.h (right): https://codereview.chromium.org/445013002/diff/520001/media/blink/webmediapla... media/blink/webmediaplayer_params.h:38: // |defer_load_cb|, |audio_renderer_sink|, and |compositor_task_runner| may be update docs that state that the context provider CB may be null https://codereview.chromium.org/445013002/diff/520001/media/blink/webmediapla... media/blink/webmediaplayer_params.h:50: const Context3DProviderCB& shared_main_thread_context_3d_provider_cb, naming suggestion: just use "context_3d_provider_cb" the "shared_main_thread" part is an implementation detail of content/renderer/ code, which media/blink/ code knows nothing about https://codereview.chromium.org/445013002/diff/520001/media/filters/context_3... File media/filters/context_3d_provider.h (right): https://codereview.chromium.org/445013002/diff/520001/media/filters/context_3... media/filters/context_3d_provider.h:18: struct Context3DProvider { docs covering basic usage https://codereview.chromium.org/445013002/diff/520001/media/filters/context_3... media/filters/context_3d_provider.h:23: typedef base::Callback<Context3DProvider()> Context3DProviderCB; this should be in WebMediaPlayerParams as it's only used there https://codereview.chromium.org/445013002/diff/520001/media/filters/skcanvas_... File media/filters/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/445013002/diff/520001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:246: SkGrPixelRef* pixelRef = SkNEW_ARGS(SkGrPixelRef, (info, texture.get())); pixelRef -> pixel_ref https://codereview.chromium.org/445013002/diff/520001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:255: VideoFrame* video_frame, const scoped_refptr<VideoFrame>& https://codereview.chromium.org/445013002/diff/520001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:272: // If CopyVideoFrameToTexture() changes the state of the |texture_id|, it's "CopyVideoFrameToTexture" doesn't refer to a function that exists I believe you mean CopyVideoFrameTextureToGLTexture() https://codereview.chromium.org/445013002/diff/520001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:289: virtual ~SyncPointClientImpl() {} s/virtual/override/ everywhere https://codereview.chromium.org/445013002/diff/520001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:299: }; DISALLOW_COPY_AND_ASSIGN() https://codereview.chromium.org/445013002/diff/520001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:443: DCHECK(context_provider.gl && context_provider.gr_context); nit: break out into two DCHECKs so we know which one was NULL if they fail https://codereview.chromium.org/445013002/diff/520001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:445: if (!CopyVideoFrameTextureToSkBitmapTexture(video_frame.get(), you can drop the .get() if you switch to using const scoped_refptr<VideoFrame>& as suggested above https://codereview.chromium.org/445013002/diff/520001/media/filters/skcanvas_... File media/filters/skcanvas_video_renderer.h (right): https://codereview.chromium.org/445013002/diff/520001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.h:46: static void CopyVideoFrameTextureToGLTexture(gpu::gles2::GLES2Interface* gl, can we get documentation? for example, it'd be good to highlight that this requires NATIVE_TEXTURE https://codereview.chromium.org/445013002/diff/520001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.h:47: media::VideoFrame* video_frame, nit: typically we pass these as const-ref (you can also drop the media:: prefix) const scoped_refptr<VideoFrame>& video_frame
danakj@chromium.org changed reviewers: + danakj@chromium.org
https://codereview.chromium.org/445013002/diff/520001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/445013002/diff/520001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:544: (video_frame.get() && On 2014/10/30 20:40:11, scherkus wrote: > nit: is the .get() needed? It is for scoped_refptr for now, which this is. But not for scoped_ptr, which makes this seem inconsistent. scoped_refptr doesn't have a bool-like operator yet. (Part of transition to get rid of implicit T* conversions.)
danakj@chromium.org changed reviewers: - danakj@chromium.org
https://codereview.chromium.org/445013002/diff/520001/media/filters/skcanvas_... File media/filters/skcanvas_video_renderer.h (right): https://codereview.chromium.org/445013002/diff/520001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.h:47: media::VideoFrame* video_frame, On 2014/10/30 20:40:13, scherkus wrote: > nit: typically we pass these as const-ref (you can also drop the media:: prefix) > > const scoped_refptr<VideoFrame>& video_frame If you don't need a reference, passing a T* is more clear that you are not passing something that you can take a reference on. I would recommend this over const scoped_refptr<T>& when you don't need to take a ref on T.
Thank you for very detailed review. I get rid of half of redundant .get() but preserves half of necessary .get(). https://codereview.chromium.org/445013002/diff/520001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/445013002/diff/520001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:544: (video_frame.get() && On 2014/10/30 21:02:52, danakj wrote: > On 2014/10/30 20:40:11, scherkus wrote: > > nit: is the .get() needed? > > It is for scoped_refptr for now, which this is. But not for scoped_ptr, which > makes this seem inconsistent. scoped_refptr doesn't have a bool-like operator > yet. (Part of transition to get rid of implicit T* conversions.) danakj, thank you for answering. scoped_refptr::operator T*() is defined in the strange guard and in linux it's disabled. https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/ref_co... https://codereview.chromium.org/445013002/diff/520001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:599: video_frame.get(), On 2014/10/30 20:40:11, scherkus wrote: > nit: is .get() needed? not needed. done. https://codereview.chromium.org/445013002/diff/520001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/445013002/diff/520001/content/renderer/render... content/renderer/render_frame_impl.cc:480: DCHECK(render_thread); On 2014/10/30 20:40:11, scherkus wrote: > nit: I'm not sure how useful these DCHECKs are in practice as current() will > return nullptr if you don't access this on the right thread (it relies on TLS) > > we also dereference on line 483 -- so we're essentially checking for the same > condition multiple times in a variety of different ways I agree. I removed DCHECKs. https://codereview.chromium.org/445013002/diff/520001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/445013002/diff/520001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:524: (video_frame.get() && On 2014/10/30 20:40:11, scherkus wrote: > nit: is .get() needed? needed. https://codereview.chromium.org/445013002/diff/520001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:533: skcanvas_video_renderer_.Paint(video_frame.get(), On 2014/10/30 20:40:11, scherkus wrote: > nit: is .get() needed? not needed. https://codereview.chromium.org/445013002/diff/520001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:599: if (!video_frame.get() || On 2014/10/30 20:40:11, scherkus wrote: > nit: is .get() needed? needed https://codereview.chromium.org/445013002/diff/520001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:610: video_frame.get(), On 2014/10/30 20:40:11, scherkus wrote: > nit: is .get() needed? needed https://codereview.chromium.org/445013002/diff/520001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/445013002/diff/520001/media/blink/webmediapla... media/blink/webmediaplayer_impl.h:275: Context3DProviderCB shared_main_thread_context_3d_provider_cb_; On 2014/10/30 20:40:11, scherkus wrote: > ditto for naming: drop the the "shared_main_thread_" part Done. https://codereview.chromium.org/445013002/diff/520001/media/blink/webmediapla... File media/blink/webmediaplayer_params.h (right): https://codereview.chromium.org/445013002/diff/520001/media/blink/webmediapla... media/blink/webmediaplayer_params.h:38: // |defer_load_cb|, |audio_renderer_sink|, and |compositor_task_runner| may be On 2014/10/30 20:40:11, scherkus wrote: > update docs that state that the context provider CB may be null Thank you for checking. Done. https://codereview.chromium.org/445013002/diff/520001/media/blink/webmediapla... media/blink/webmediaplayer_params.h:50: const Context3DProviderCB& shared_main_thread_context_3d_provider_cb, On 2014/10/30 20:40:12, scherkus wrote: > naming suggestion: just use "context_3d_provider_cb" > > the "shared_main_thread" part is an implementation detail of content/renderer/ > code, which media/blink/ code knows nothing about That's right! Done. https://codereview.chromium.org/445013002/diff/520001/media/filters/context_3... File media/filters/context_3d_provider.h (right): https://codereview.chromium.org/445013002/diff/520001/media/filters/context_3... media/filters/context_3d_provider.h:18: struct Context3DProvider { On 2014/10/30 20:40:12, scherkus wrote: > docs covering basic usage Done. https://codereview.chromium.org/445013002/diff/520001/media/filters/context_3... media/filters/context_3d_provider.h:23: typedef base::Callback<Context3DProvider()> Context3DProviderCB; On 2014/10/30 20:40:12, scherkus wrote: > this should be in WebMediaPlayerParams as it's only used there Done. https://codereview.chromium.org/445013002/diff/520001/media/filters/skcanvas_... File media/filters/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/445013002/diff/520001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:246: SkGrPixelRef* pixelRef = SkNEW_ARGS(SkGrPixelRef, (info, texture.get())); On 2014/10/30 20:40:12, scherkus wrote: > pixelRef -> pixel_ref oops, Thank you. https://codereview.chromium.org/445013002/diff/520001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:255: VideoFrame* video_frame, On 2014/10/30 20:40:13, scherkus wrote: > const scoped_refptr<VideoFrame>& not needed because it doesn't take a ref https://codereview.chromium.org/445013002/diff/520001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:272: // If CopyVideoFrameToTexture() changes the state of the |texture_id|, it's On 2014/10/30 20:40:12, scherkus wrote: > "CopyVideoFrameToTexture" doesn't refer to a function that exists > > I believe you mean CopyVideoFrameTextureToGLTexture() Thank you! https://codereview.chromium.org/445013002/diff/520001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:289: virtual ~SyncPointClientImpl() {} On 2014/10/30 20:40:12, scherkus wrote: > s/virtual/override/ everywhere interesting. Done. https://codereview.chromium.org/445013002/diff/520001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:299: }; On 2014/10/30 20:40:12, scherkus wrote: > DISALLOW_COPY_AND_ASSIGN() Done. https://codereview.chromium.org/445013002/diff/520001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:443: DCHECK(context_provider.gl && context_provider.gr_context); On 2014/10/30 20:40:13, scherkus wrote: > nit: break out into two DCHECKs so we know which one was NULL if they fail Done. https://codereview.chromium.org/445013002/diff/520001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:445: if (!CopyVideoFrameTextureToSkBitmapTexture(video_frame.get(), On 2014/10/30 20:40:13, scherkus wrote: > you can drop the .get() if you switch to using const scoped_refptr<VideoFrame>& > as suggested above CopyVideoFrameTextureToSkBitmapTexture doesn't take a ref, so I preserve it. https://codereview.chromium.org/445013002/diff/520001/media/filters/skcanvas_... File media/filters/skcanvas_video_renderer.h (right): https://codereview.chromium.org/445013002/diff/520001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.h:46: static void CopyVideoFrameTextureToGLTexture(gpu::gles2::GLES2Interface* gl, On 2014/10/30 20:40:13, scherkus wrote: > can we get documentation? > > for example, it'd be good to highlight that this requires NATIVE_TEXTURE Done. https://codereview.chromium.org/445013002/diff/520001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.h:47: media::VideoFrame* video_frame, On 2014/10/30 21:04:20, danakj wrote: > On 2014/10/30 20:40:13, scherkus wrote: > > nit: typically we pass these as const-ref (you can also drop the media:: > prefix) > > > > const scoped_refptr<VideoFrame>& video_frame > > If you don't need a reference, passing a T* is more clear that you are not > passing something that you can take a reference on. I would recommend this over > const scoped_refptr<T>& when you don't need to take a ref on T. I preserve T* because it doesn't need to take a ref. Paint() also has used T* before introducing VideoImageGenerator, which takes a ref.
Hi scherkus@, could you review again?
lgtm w/ nit https://codereview.chromium.org/445013002/diff/540001/media/filters/skcanvas_... File media/filters/skcanvas_video_renderer.h (right): https://codereview.chromium.org/445013002/diff/540001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.h:34: // is ganeshed, |context_provider| must be provided. can we DCHECK this inside the .cc?
On 2014/11/13 18:35:02, scherkus wrote: > lgtm w/ nit Thank you for review! Done. jamesr@, could you review content/renderer/ and mojo/ ? > https://codereview.chromium.org/445013002/diff/540001/media/filters/skcanvas_... > File media/filters/skcanvas_video_renderer.h (right): > > https://codereview.chromium.org/445013002/diff/540001/media/filters/skcanvas_... > media/filters/skcanvas_video_renderer.h:34: // is ganeshed, |context_provider| > must be provided. > can we DCHECK this inside the .cc? I check it in .cc already, but I changes new patch set to check it more explicitly.
Patchset #12 (id:560001) has been deleted
It looks like this patch still has some component build failures on the trybots - could you green those up so I can make sure I'm reviewing what'll land? https://codereview.chromium.org/445013002/diff/580001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/445013002/diff/580001/content/renderer/render... content/renderer/render_frame_impl.cc:484: return {nullptr, nullptr}; please don't use this initialization syntax https://codereview.chromium.org/445013002/diff/580001/content/renderer/render... content/renderer/render_frame_impl.cc:490: int32) = nullptr; could you use a type alias to make this more readable?
https://codereview.chromium.org/445013002/diff/580001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/445013002/diff/580001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:587: // TODO(dshwang): need more elegant way to convert WebGraphicsContext3D to this is really gross. why do you want a WebGraphicsContext3D anyway?
On 2014/11/13 20:39:17, jamesr wrote: > It looks like this patch still has some component build failures on the trybots > - could you green those up so I can make sure I'm reviewing what'll land? Thank you for review. I fixes invalid DCHECK, which WebRTC unittests uses SkCanvasVideoRenderer::Paint with non-Ganesh SkCanvas and HW VideoFrame. I change the condition of SkCanvasVideoRenderer::Paint, which require context_3d_provider if Ganesh SkCanvas _and_ HW VideoFrame, not or. > https://codereview.chromium.org/445013002/diff/580001/content/renderer/render... > File content/renderer/render_frame_impl.cc (right): > > https://codereview.chromium.org/445013002/diff/580001/content/renderer/render... > content/renderer/render_frame_impl.cc:484: return {nullptr, nullptr}; > please don't use this initialization syntax DONE. > https://codereview.chromium.org/445013002/diff/580001/content/renderer/render... > content/renderer/render_frame_impl.cc:490: int32) = nullptr; > could you use a type alias to make this more readable? Done. > https://codereview.chromium.org/445013002/diff/580001/content/renderer/media/... > File content/renderer/media/android/webmediaplayer_android.cc (right): > > https://codereview.chromium.org/445013002/diff/580001/content/renderer/media/... > content/renderer/media/android/webmediaplayer_android.cc:587: // TODO(dshwang): > need more elegant way to convert WebGraphicsContext3D to > this is really gross. why do you want a WebGraphicsContext3D anyway? I thought media/filter should not depend of WebKit/public/platform, but as you ask it, I guess media/filter can depend on WebKit/public/platform. New patch set makes SkCanvasVideoRenderer use WebGraphicsContext3D, not GLES2Interface.
Patchset #13 (id:600001) has been deleted
https://codereview.chromium.org/445013002/diff/620001/media/DEPS File media/DEPS (right): https://codereview.chromium.org/445013002/diff/620001/media/DEPS#newcode11 media/DEPS:11: "+third_party/WebKit/public/platform", no. only media/blink can depend on blink types
On 2014/11/14 16:54:32, jamesr wrote: > https://codereview.chromium.org/445013002/diff/620001/media/DEPS > File media/DEPS (right): > > https://codereview.chromium.org/445013002/diff/620001/media/DEPS#newcode11 > media/DEPS:11: "+third_party/WebKit/public/platform", > no. > > only media/blink can depend on blink types That means we choose either option 1. allow gross casting from WebGraphicsContext3D to GLES2Interface 2. write duplicated code for both WebGraphicsContext3D and GLES2Interface Which do you want? > https://codereview.chromium.org/445013002/diff/580001/content/renderer/media/... > > content/renderer/media/android/webmediaplayer_android.cc:587: // > TODO(dshwang): > > need more elegant way to convert WebGraphicsContext3D to > > this is really gross. why do you want a WebGraphicsContext3D anyway?
I still don't understand why WebGraphicsContext3D is involved at all here. Please justify.
On 2014/11/14 18:41:54, jamesr wrote: > I still don't understand why WebGraphicsContext3D is involved at all here. > Please justify. Ah, yes. WebMediaPlayer has following API, which webgl requests chromium to copy video frame to given texture using the |WebGraphicsContext3D| instance, which webgl owns. WebMediaPlayer::copyVideoTextureToPlatformTexture(blink::WebGraphicsContext3D*, ...); WebMediaPlayer::paint(SkCanvas, ...) is used by 2d canvas or blink renderer to draw video frame on the given |SkCanvas|. This CL accelerates WebMediaPlayer::paint in the case, which both SkCanvas and VideoFrame are accelerated. In the case, it needs very similar code to copyVideoTextureToPlatformTexture() in media/filter. So I reuse the duplicated code in media/filter/ for copyVideoTextureToPlatformTexture() via gross converting. In summary, this CL implements similar code in media/filter/ and I didn't want to write duplicated code to WebMediaPlayer::copyVideoTextureToPlatformTexture().
hi, could you have a chance to look at it again? Thanks.
jamesr@, could you have a chance to look at it again? Thanks.
Hi jamesr@, could you have a chance to look at it again? Thanks.
It feels really awkward to provide the context itself via a callback. We already have a delegate interface between media/blink/webmediaplayer_impl and content/. We also have a GpuVideoAcceleratorFactories mechanism for a similar thing. Can we reuse one of those concepts instead of making up another one with different semantics? This is really confusing. https://codereview.chromium.org/445013002/diff/660001/media/filters/context_3... File media/filters/context_3d_provider.h (right): https://codereview.chromium.org/445013002/diff/660001/media/filters/context_3... media/filters/context_3d_provider.h:25: struct Context3DProvider { cc::ContextProvider exists to provide a shared ownership model of the context. this appears to just have raw pointers (i.e. no ownership) of the underlying gl and gr contexts. I'm not sure what value bundling the two pointers together provides, but whatever it is it should definitely not have a name so similar to cc::ContextProvider since it doesn't provide the same lifetime properties.
On 2014/11/24 22:19:50, jamesr wrote: > It feels really awkward to provide the context itself via a callback. We > already have a delegate interface between media/blink/webmediaplayer_impl and > content/. We also have a GpuVideoAcceleratorFactories mechanism for a similar > thing. Can we reuse one of those concepts instead of making up another one with > different semantics? This is really confusing. Thank you for your opinion. I explain more why I made it. The goal is to provide SkCanvasVideoRenderer GLES2Interface and GrContext (based on shared mainthread 3d context) to SkCanvasVideoRenderer. GpuVideoAcceleratorFactories exists to create new 3d context for media thread. Creating new 3d context for only SkCanvasVideoRenderer seems to be overkill, so I reuse the shared context. I think WebMediaPlayerDelegate is "a delegate interface between media/blink/webmediaplayer_impl and content/" which you mentioned. WebMediaPlayerDelegate plays a role to communicate changes of play state, while WebMediaPlayerParams already transfer gpu_factories_ from content to media. I thought WebMediaPlayerParams is more right place to transfer Context3D callback. Speaking callback, shared mainthread 3d context can be removed anytime because of gpu crash. So it's not enough to transfer the pointer of the 3d context at the webmediaplayer_impl creation time. SkCanvasVideoRenderer want to get real-time RenderThreadImpl::SharedMainThreadContextProvider(). It's why I made this sorta complicated callback mechanism. > https://codereview.chromium.org/445013002/diff/660001/media/filters/context_3... > File media/filters/context_3d_provider.h (right): > > https://codereview.chromium.org/445013002/diff/660001/media/filters/context_3... > media/filters/context_3d_provider.h:25: struct Context3DProvider { > cc::ContextProvider exists to provide a shared ownership model of the context. > this appears to just have raw pointers (i.e. no ownership) of the underlying gl > and gr contexts. I'm not sure what value bundling the two pointers together > provides, but whatever it is it should definitely not have a name so similar to > cc::ContextProvider since it doesn't provide the same lifetime properties. Yes, agree. I rename Context3DProvider to Context3D. SkCanvasVideoRenderer creates ganesh SkBitmap and copy texture, so it needs both GLES2Interface and GrContext. cc::ContextProvider can not be used in media/ because of cyclic dependency.
Patchset #16 (id:680001) has been deleted
On 2014/11/25 09:49:27, dshwang wrote: > On 2014/11/24 22:19:50, jamesr wrote: > > It feels really awkward to provide the context itself via a callback. We > > already have a delegate interface between media/blink/webmediaplayer_impl and > > content/. We also have a GpuVideoAcceleratorFactories mechanism for a similar > > thing. Can we reuse one of those concepts instead of making up another one > with > > different semantics? This is really confusing. > > Thank you for your opinion. I explain more why I made it. > The goal is to provide SkCanvasVideoRenderer GLES2Interface and GrContext (based > on shared mainthread 3d context) to SkCanvasVideoRenderer. > GpuVideoAcceleratorFactories exists to create new 3d context for media thread. > Creating new 3d context for only SkCanvasVideoRenderer seems to be overkill, so > I reuse the shared context. > I think WebMediaPlayerDelegate is "a delegate interface between > media/blink/webmediaplayer_impl and content/" which you mentioned. > WebMediaPlayerDelegate plays a role to communicate changes of play state, while > WebMediaPlayerParams already transfer gpu_factories_ from content to media. > I thought WebMediaPlayerParams is more right place to transfer Context3D > callback. > Speaking callback, shared mainthread 3d context can be removed anytime because > of gpu crash. So it's not enough to transfer the pointer of the 3d context at > the webmediaplayer_impl creation time. > SkCanvasVideoRenderer want to get real-time > RenderThreadImpl::SharedMainThreadContextProvider(). It's why I made this sorta > complicated callback mechanism. > > > > https://codereview.chromium.org/445013002/diff/660001/media/filters/context_3... > > File media/filters/context_3d_provider.h (right): > > > > > https://codereview.chromium.org/445013002/diff/660001/media/filters/context_3... > > media/filters/context_3d_provider.h:25: struct Context3DProvider { > > cc::ContextProvider exists to provide a shared ownership model of the context. > > > this appears to just have raw pointers (i.e. no ownership) of the underlying > gl > > and gr contexts. I'm not sure what value bundling the two pointers together > > provides, but whatever it is it should definitely not have a name so similar > to > > cc::ContextProvider since it doesn't provide the same lifetime properties. > > Yes, agree. I rename Context3DProvider to Context3D. > SkCanvasVideoRenderer creates ganesh SkBitmap and copy texture, so it needs both > GLES2Interface and GrContext. > cc::ContextProvider can not be used in media/ because of cyclic dependency. @jamesr, could I listen to your opinion again?
@jamesr, could I listen to your opinion again? Thank you in advance. On 2014/12/03 10:14:13, dshwang wrote: > On 2014/11/25 09:49:27, dshwang wrote: > > On 2014/11/24 22:19:50, jamesr wrote: > > > It feels really awkward to provide the context itself via a callback. We > > > already have a delegate interface between media/blink/webmediaplayer_impl > and > > > content/. We also have a GpuVideoAcceleratorFactories mechanism for a > similar > > > thing. Can we reuse one of those concepts instead of making up another one > > with > > > different semantics? This is really confusing. > > > > Thank you for your opinion. I explain more why I made it. > > The goal is to provide SkCanvasVideoRenderer GLES2Interface and GrContext > (based > > on shared mainthread 3d context) to SkCanvasVideoRenderer. > > GpuVideoAcceleratorFactories exists to create new 3d context for media thread. > > Creating new 3d context for only SkCanvasVideoRenderer seems to be overkill, > so > > I reuse the shared context. > > I think WebMediaPlayerDelegate is "a delegate interface between > > media/blink/webmediaplayer_impl and content/" which you mentioned. > > WebMediaPlayerDelegate plays a role to communicate changes of play state, > while > > WebMediaPlayerParams already transfer gpu_factories_ from content to media. > > I thought WebMediaPlayerParams is more right place to transfer Context3D > > callback. > > Speaking callback, shared mainthread 3d context can be removed anytime because > > of gpu crash. So it's not enough to transfer the pointer of the 3d context at > > the webmediaplayer_impl creation time. > > SkCanvasVideoRenderer want to get real-time > > RenderThreadImpl::SharedMainThreadContextProvider(). It's why I made this > sorta > > complicated callback mechanism. > > > > > > > > https://codereview.chromium.org/445013002/diff/660001/media/filters/context_3... > > > File media/filters/context_3d_provider.h (right): > > > > > > > > > https://codereview.chromium.org/445013002/diff/660001/media/filters/context_3... > > > media/filters/context_3d_provider.h:25: struct Context3DProvider { > > > cc::ContextProvider exists to provide a shared ownership model of the > context. > > > > > this appears to just have raw pointers (i.e. no ownership) of the underlying > > gl > > > and gr contexts. I'm not sure what value bundling the two pointers together > > > provides, but whatever it is it should definitely not have a name so similar > > to > > > cc::ContextProvider since it doesn't provide the same lifetime properties. > > > > Yes, agree. I rename Context3DProvider to Context3D. > > SkCanvasVideoRenderer creates ganesh SkBitmap and copy texture, so it needs > both > > GLES2Interface and GrContext. > > cc::ContextProvider can not be used in media/ because of cyclic dependency. > > @jamesr, could I listen to your opinion again?
jamesr@chromium.org changed reviewers: - jamesr@chromium.org
I'm not working on this code any more.
On 2014/12/09 19:39:21, jamesr wrote: > I'm not working on this code any more. jamesr: considering you left feedback ~3 weeks ago that caused this CL to change, could you at least nominate someone else to review the parts you care about?
jamesr@chromium.org changed reviewers: + jamesr@chromium.org
Any content owner, I suppose.
scherkus@chromium.org changed reviewers: + creis@chromium.org - jamesr@chromium.org
swapping jamesr -> creis for content/renderer/* OWNERS review this is still lgtm from me
dongseong.hwang@intel.com changed reviewers: + ben@chromium.org
On 2014/12/09 20:11:33, scherkus wrote: > swapping jamesr -> creis for content/renderer/* OWNERS review > > this is still lgtm from me Thank you for coordinating! @creis, could you review content/renderer/render_frame_impl.cc ? @ben, could you review mojo/services/html_viewer/webmediaplayer_factory.cc ?
creis@chromium.org changed reviewers: + sievers@chromium.org
I'm afraid I'm not very familiar with any of this, so the best I could offer is a rubber stamp for render_frame_impl.cc. I think @sievers is a content owner who knows about rendering on Android. Maybe he's a better reviewer for content/renderer/*, after James's earlier questions? https://codereview.chromium.org/445013002/diff/700001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/445013002/diff/700001/content/renderer/render... content/renderer/render_frame_impl.cc:488: using CreateRenderFrameImpl = RenderFrameImpl* (*)(RenderViewImpl*, int32); Is there a reason for this to be in this CL? Cleanup is generally fine, but this seems like a large enough CL already, without adding unrelated things to it. I'd suggest doing it in a separate CL, since it should probably be accompanied by updates to InstallCreateHook below. (Same with the static removals above, though those are trivial enough that I don't feel strongly.)
On 2014/12/09 22:08:23, Charlie Reis wrote: > I'm afraid I'm not very familiar with any of this, so the best I could offer is > a rubber stamp for render_frame_impl.cc. Thank you for reviewing. > I think @sievers is a content owner who knows about rendering on Android. Maybe > he's a better reviewer for content/renderer/*, after James's earlier questions? @sievers, could you review content/renderer/* ? James asked two questions Q1> why is there ugly static_cast of WebGraphicsContext3D? A> This CL needs same code for both WebGraphicsContext3D and GLES2Interface. I did to reduce duplicated code. Q2> is there more succinct way for content to give 3d context to media? A> shared offscreen context can be re-created anytime (e.g. gpu process crash) So we need callback mechanism. In more detail, I copy&paste previous discussion. https://codereview.chromium.org/445013002/#msg45 On 2014/11/14 18:41:54, jamesr wrote: > I still don't understand why WebGraphicsContext3D is involved at all here. > Please justify. Ah, yes. WebMediaPlayer has following API, which webgl requests chromium to copy video frame to given texture using the |WebGraphicsContext3D| instance, which webgl owns. WebMediaPlayer::copyVideoTextureToPlatformTexture(blink::WebGraphicsContext3D*, ...); WebMediaPlayer::paint(SkCanvas, ...) is used by 2d canvas or blink renderer to draw video frame on the given |SkCanvas|. This CL accelerates WebMediaPlayer::paint in the case, which both SkCanvas and VideoFrame are accelerated. In the case, it needs very similar code to copyVideoTextureToPlatformTexture() in media/filter. So I reuse the duplicated code in media/filter/ for copyVideoTextureToPlatformTexture() via gross converting. In summary, this CL implements similar code in media/filter/ and I didn't want to write duplicated code to WebMediaPlayer::copyVideoTextureToPlatformTexture(). https://codereview.chromium.org/445013002/#msg50 On 2014/11/24 22:19:50, jamesr wrote: > It feels really awkward to provide the context itself via a callback. We > already have a delegate interface between media/blink/webmediaplayer_impl and > content/. We also have a GpuVideoAcceleratorFactories mechanism for a similar > thing. Can we reuse one of those concepts instead of making up another one with > different semantics? This is really confusing. Thank you for your opinion. I explain more why I made it. The goal is to provide SkCanvasVideoRenderer GLES2Interface and GrContext (based on shared mainthread 3d context) to SkCanvasVideoRenderer. GpuVideoAcceleratorFactories exists to create new 3d context for media thread. Creating new 3d context for only SkCanvasVideoRenderer seems to be overkill, so I reuse the shared context. I think WebMediaPlayerDelegate is "a delegate interface between media/blink/webmediaplayer_impl and content/" which you mentioned. WebMediaPlayerDelegate plays a role to communicate changes of play state, while WebMediaPlayerParams already transfer gpu_factories_ from content to media. I thought WebMediaPlayerParams is more right place to transfer Context3D callback. Speaking callback, shared mainthread 3d context can be removed anytime because of gpu crash. So it's not enough to transfer the pointer of the 3d context at the webmediaplayer_impl creation time. SkCanvasVideoRenderer want to get real-time RenderThreadImpl::SharedMainThreadContextProvider(). It's why I made this sorta complicated callback mechanism. https://codereview.chromium.org/445013002/diff/700001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/445013002/diff/700001/content/renderer/render... content/renderer/render_frame_impl.cc:488: using CreateRenderFrameImpl = RenderFrameImpl* (*)(RenderViewImpl*, int32); On 2014/12/09 22:08:23, Charlie Reis wrote: > Is there a reason for this to be in this CL? Cleanup is generally fine, but > this seems like a large enough CL already, without adding unrelated things to > it. I'd suggest doing it in a separate CL, since it should probably be > accompanied by updates to InstallCreateHook below. > > (Same with the static removals above, though those are trivial enough that I > don't feel strongly.) @jamesr requested it in https://codereview.chromium.org/445013002/#msg38 That's good idea. I separate the clean up in https://codereview.chromium.org/787333003/
I think it might not be helpful to unify the CopyTextureCHROMIUM() path for Android with the other platforms. It comes down to limitations of SurfaceTexture. It cannot be used properly in share groups. Furthermore there is no way to query the size from a SurfaceTexture service-side and no mechanism in GLES2 to do that for textures in general either. Because of those limitations and in order to make this codepath work for Android WebView with its different sharing model, it will require using the StreamTextureFactory context and the provided web context in a non-intuitive way to make sure the texture can be copied. And then probably also setting up a custom blit rather than using CopyTexture and removing the SetSize hack (which attempts to communicate the size from the video stream as the texture size to the service; but really this is client metadata and should only be used there).
On 2014/12/10 19:58:04, sievers wrote: > I think it might not be helpful to unify the CopyTextureCHROMIUM() path for > Android with the other platforms. Thank you for reviewing. I still believe unification is good. Could I listen to your opinion again? > It comes down to limitations of SurfaceTexture. It cannot be used properly in > share groups. Furthermore there is no way to query the size from a > SurfaceTexture service-side and no mechanism in GLES2 to do that for textures in > general either. This CL doesn't change how Android code uses GL except for SkBitmap cache. This CL just move Android code to SkCanvasVideoRenderer. both previous code and this CL considers StreamTextureFactory context and 3d context used in this CL are not in the same shared group. It's why SkCanvasVideoRenderer accesses the texture via mailbox as previous code did. WebMidiaPlayerAndroid keeps the texture size in VideoFrame and SkCanvasVideoRenderer can know the size via video_frame->visible_rect(), instead of querying it to SurfaceTexture service-side. > Because of those limitations and in order to make this codepath work for Android > WebView with its different sharing model, it will require using the > StreamTextureFactory context and the provided web context in a non-intuitive way > to make sure the texture can be copied. The relationship between StreamTextureFactory context in chrome_shell/chrome and shared context is same to the relationship between StreamTextureFactory context in "WebView" and shared context. Both context of both cases are different shared group. So SkCanvasVideoRenderer already accesses the texture via mailbox. WebView requirement is not different from chrome_shell. > And then probably also setting up a > custom blit rather than using CopyTexture and removing the SetSize hack (which > attempts to communicate the size from the video stream as the texture size to > the service; but really this is client metadata and should only be used there). Could you explain more? Do you mean that we can not trust video_frame->visible_rect() in WebView? If so, many existing code are also wrong.
On 2014/12/10 20:25:45, dshwang wrote: > On 2014/12/10 19:58:04, sievers wrote: > > I think it might not be helpful to unify the CopyTextureCHROMIUM() path for > > Android with the other platforms. > > Thank you for reviewing. I still believe unification is good. Could I listen to > your opinion again? > > > It comes down to limitations of SurfaceTexture. It cannot be used properly in > > share groups. Furthermore there is no way to query the size from a > > SurfaceTexture service-side and no mechanism in GLES2 to do that for textures > in > > general either. > > This CL doesn't change how Android code uses GL except for SkBitmap cache. This > CL just move Android code to SkCanvasVideoRenderer. > both previous code and this CL considers StreamTextureFactory context and 3d > context used in this CL are not in the same shared group. > It's why SkCanvasVideoRenderer accesses the texture via mailbox as previous code > did. > WebMidiaPlayerAndroid keeps the texture size in VideoFrame and > SkCanvasVideoRenderer can know the size via video_frame->visible_rect(), instead > of querying it to SurfaceTexture service-side. > > > Because of those limitations and in order to make this codepath work for > Android > > WebView with its different sharing model, it will require using the > > StreamTextureFactory context and the provided web context in a non-intuitive > way > > to make sure the texture can be copied. > > The relationship between StreamTextureFactory context in chrome_shell/chrome and > shared context is same to the relationship between StreamTextureFactory context > in "WebView" and shared context. Both context of both cases are different shared > group. So SkCanvasVideoRenderer already accesses the texture via mailbox. > WebView requirement is not different from chrome_shell. > > > And then probably also setting up a > > custom blit rather than using CopyTexture and removing the SetSize hack (which > > attempts to communicate the size from the video stream as the texture size to > > the service; but really this is client metadata and should only be used > there). > > Could you explain more? Do you mean that we can not trust > video_frame->visible_rect() in WebView? If so, many existing code are also > wrong. That's the visible destination size for compositing. |natural_size_| comes from the video stream metadata I think. So it's the size that the decoded texture should be. But since we are dealing with a buffer queue and updateTexImage() service-side, this is really hard to synchronize should it ever change. Currently SetStreamTextureSize() is used to send an IPC to the decoder which then sets it as the GLImage's size. I think this is questionable, since one comes from the video stream and not GL. We should leave it to the client to set up the blit with what it *thinks* is the size. (Also this SetStreamTextureSize plumbing is not there for WebView.) The sharing issue (for WebView) is another thing. It will require doing this copy/blit using two contexts in a non-intuitive way. (It's complicated to explain.) We'll have to then run the webgl tests on the bots to make sure this code doesn't regress. So I'm just saying that any changes you make to webmediaplayer_android.cc we will most likely have to revert again. It's very unfortunate, but essentially this requires some Android-specific hackery and therefore won't allow for code unification with the other platforms.
On 2014/12/10 21:44:10, sievers wrote: > On 2014/12/10 20:25:45, dshwang wrote: > > On 2014/12/10 19:58:04, sievers wrote: > > > I think it might not be helpful to unify the CopyTextureCHROMIUM() path for > > > Android with the other platforms. > > > > Thank you for reviewing. I still believe unification is good. Could I listen > to > > your opinion again? > > > > > It comes down to limitations of SurfaceTexture. It cannot be used properly > in > > > share groups. Furthermore there is no way to query the size from a > > > SurfaceTexture service-side and no mechanism in GLES2 to do that for > textures > > in > > > general either. > > > > This CL doesn't change how Android code uses GL except for SkBitmap cache. > This > > CL just move Android code to SkCanvasVideoRenderer. > > both previous code and this CL considers StreamTextureFactory context and 3d > > context used in this CL are not in the same shared group. > > It's why SkCanvasVideoRenderer accesses the texture via mailbox as previous > code > > did. > > WebMidiaPlayerAndroid keeps the texture size in VideoFrame and > > SkCanvasVideoRenderer can know the size via video_frame->visible_rect(), > instead > > of querying it to SurfaceTexture service-side. > > > > > Because of those limitations and in order to make this codepath work for > > Android > > > WebView with its different sharing model, it will require using the > > > StreamTextureFactory context and the provided web context in a non-intuitive > > way > > > to make sure the texture can be copied. > > > > The relationship between StreamTextureFactory context in chrome_shell/chrome > and > > shared context is same to the relationship between StreamTextureFactory > context > > in "WebView" and shared context. Both context of both cases are different > shared > > group. So SkCanvasVideoRenderer already accesses the texture via mailbox. > > WebView requirement is not different from chrome_shell. > > > > > And then probably also setting up a > > > custom blit rather than using CopyTexture and removing the SetSize hack > (which > > > attempts to communicate the size from the video stream as the texture size > to > > > the service; but really this is client metadata and should only be used > > there). > > > > Could you explain more? Do you mean that we can not trust > > video_frame->visible_rect() in WebView? If so, many existing code are also > > wrong. > > That's the visible destination size for compositing. |natural_size_| comes from > the video stream metadata I think. > So it's the size that the decoded texture should be. But since we are dealing > with a buffer queue and updateTexImage() service-side, this is really hard to > synchronize should it ever change. > Currently SetStreamTextureSize() is used to send an IPC to the decoder which > then sets it as the GLImage's size. I think this is questionable, since one > comes from the video stream and not GL. We should leave it to the client to set > up the blit with what it *thinks* is the size. (Also this SetStreamTextureSize > plumbing is not there for WebView.) > > The sharing issue (for WebView) is another thing. It will require doing this > copy/blit using two contexts in a non-intuitive way. (It's complicated to > explain.) We'll have to then run the webgl tests on the bots to make sure this > code doesn't regress. > > So I'm just saying that any changes you make to webmediaplayer_android.cc we > will most likely have to revert again. > It's very unfortunate, but essentially this requires some Android-specific > hackery and therefore won't allow for code unification with the other platforms. Thank you for patient explanation. I rollback Android code as you suggested. When WebView video were to be stable, I'll unify code if possible. https://codereview.chromium.org/445013002/diff/740001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/445013002/diff/740001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:89: bool IsSkBitmapProperlySizedTexture(const SkBitmap* bitmap, EnsureTextureBackedSkBitmap was separated into two functions to improve readability in SkCanvasVideoRenderer. I applied it to here in order to unify the code easily in the future if possible. https://codereview.chromium.org/445013002/diff/740001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:101: // RGBA to BGRA conversion. Android can use kSkia8888_GrPixelConfig because RGBA is default format. However, I copy this code from SkCanvasVideoRenderer in order to unify the code easily in the future if possible.
On 2014/12/12 17:06:59, dshwang wrote: > On 2014/12/10 21:44:10, sievers wrote: > > On 2014/12/10 20:25:45, dshwang wrote: > > > On 2014/12/10 19:58:04, sievers wrote: > > > > I think it might not be helpful to unify the CopyTextureCHROMIUM() path > for > > > > Android with the other platforms. > > > > > > Thank you for reviewing. I still believe unification is good. Could I listen > > to > > > your opinion again? > > > > > > > It comes down to limitations of SurfaceTexture. It cannot be used properly > > in > > > > share groups. Furthermore there is no way to query the size from a > > > > SurfaceTexture service-side and no mechanism in GLES2 to do that for > > textures > > > in > > > > general either. > > > > > > This CL doesn't change how Android code uses GL except for SkBitmap cache. > > This > > > CL just move Android code to SkCanvasVideoRenderer. > > > both previous code and this CL considers StreamTextureFactory context and 3d > > > context used in this CL are not in the same shared group. > > > It's why SkCanvasVideoRenderer accesses the texture via mailbox as previous > > code > > > did. > > > WebMidiaPlayerAndroid keeps the texture size in VideoFrame and > > > SkCanvasVideoRenderer can know the size via video_frame->visible_rect(), > > instead > > > of querying it to SurfaceTexture service-side. > > > > > > > Because of those limitations and in order to make this codepath work for > > > Android > > > > WebView with its different sharing model, it will require using the > > > > StreamTextureFactory context and the provided web context in a > non-intuitive > > > way > > > > to make sure the texture can be copied. > > > > > > The relationship between StreamTextureFactory context in chrome_shell/chrome > > and > > > shared context is same to the relationship between StreamTextureFactory > > context > > > in "WebView" and shared context. Both context of both cases are different > > shared > > > group. So SkCanvasVideoRenderer already accesses the texture via mailbox. > > > WebView requirement is not different from chrome_shell. > > > > > > > And then probably also setting up a > > > > custom blit rather than using CopyTexture and removing the SetSize hack > > (which > > > > attempts to communicate the size from the video stream as the texture size > > to > > > > the service; but really this is client metadata and should only be used > > > there). > > > > > > Could you explain more? Do you mean that we can not trust > > > video_frame->visible_rect() in WebView? If so, many existing code are also > > > wrong. > > > > That's the visible destination size for compositing. |natural_size_| comes > from > > the video stream metadata I think. > > So it's the size that the decoded texture should be. But since we are dealing > > with a buffer queue and updateTexImage() service-side, this is really hard to > > synchronize should it ever change. > > Currently SetStreamTextureSize() is used to send an IPC to the decoder which > > then sets it as the GLImage's size. I think this is questionable, since one > > comes from the video stream and not GL. We should leave it to the client to > set > > up the blit with what it *thinks* is the size. (Also this SetStreamTextureSize > > plumbing is not there for WebView.) > > > > The sharing issue (for WebView) is another thing. It will require doing this > > copy/blit using two contexts in a non-intuitive way. (It's complicated to > > explain.) We'll have to then run the webgl tests on the bots to make sure this > > code doesn't regress. > > > > So I'm just saying that any changes you make to webmediaplayer_android.cc we > > will most likely have to revert again. > > It's very unfortunate, but essentially this requires some Android-specific > > hackery and therefore won't allow for code unification with the other > platforms. > > Thank you for patient explanation. I rollback Android code as you suggested. > When WebView video were to be stable, I'll unify code if possible. > Thanks, that sounds great. Sorry for making it harder, and thanks for cleaning this up. > https://codereview.chromium.org/445013002/diff/740001/content/renderer/media/... > File content/renderer/media/android/webmediaplayer_android.cc (right): > > https://codereview.chromium.org/445013002/diff/740001/content/renderer/media/... > content/renderer/media/android/webmediaplayer_android.cc:89: bool > IsSkBitmapProperlySizedTexture(const SkBitmap* bitmap, > EnsureTextureBackedSkBitmap was separated into two functions to improve > readability in SkCanvasVideoRenderer. I applied it to here in order to unify the > code easily in the future if possible. > > https://codereview.chromium.org/445013002/diff/740001/content/renderer/media/... > content/renderer/media/android/webmediaplayer_android.cc:101: // RGBA to BGRA > conversion. > Android can use kSkia8888_GrPixelConfig because RGBA is default format. However, > I copy this code from SkCanvasVideoRenderer in order to unify the code easily in > the future if possible.
On 2014/12/17 22:49:04, sievers wrote: > On 2014/12/12 17:06:59, dshwang wrote: > > > > Thank you for patient explanation. I rollback Android code as you suggested. > > When WebView video were to be stable, I'll unify code if possible. > > > > Thanks, that sounds great. Sorry for making it harder, and thanks for cleaning > this up. > @sievers, Could you give me lgtm for content/renderer/ ? @ben, could you review mojo/services/html_viewer/webmediaplayer_factory.cc ?
content/renderer lgtm https://codereview.chromium.org/445013002/diff/740001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/445013002/diff/740001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:101: // RGBA to BGRA conversion. On 2014/12/12 17:06:59, dshwang wrote: > Android can use kSkia8888_GrPixelConfig because RGBA is default format. However, > I copy this code from SkCanvasVideoRenderer in order to unify the code easily in > the future if possible. You probably know since you are on bug 358198, but: I think the webgl bots won't cover this path currently, since the tests are still disabled (since the bots are not on L yet). But you probably tested this :) https://codereview.chromium.org/445013002/diff/740001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:103: desc.fFlags = kRenderTarget_GrTextureFlagBit | kNoStencil_GrTextureFlagBit; while you are in here: mind putting a comment that 'kRenderTarget' here avoids a copy before readback in skia (as I recently found out)? I assume that's why it's used here, and it's not really obvious why we would need this flag since we don't render to the texture, but only read it back.
dongseong.hwang@intel.com changed reviewers: + aa@chromium.org
@sievers thank you for reviewing! @aa could you review mojo/services/html_viewer/webmediaplayer_factory.cc ? https://codereview.chromium.org/445013002/diff/740001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/445013002/diff/740001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:101: // RGBA to BGRA conversion. On 2014/12/18 23:12:31, sievers wrote: > On 2014/12/12 17:06:59, dshwang wrote: > > Android can use kSkia8888_GrPixelConfig because RGBA is default format. > However, > > I copy this code from SkCanvasVideoRenderer in order to unify the code easily > in > > the future if possible. > > You probably know since you are on bug 358198, but: I think the webgl bots won't > cover this path currently, since the tests are still disabled (since the bots > are not on L yet). But you probably tested this :) Yes, I tested both video to webgl and video to canvas 2d using real sites as well as webgl CTS. http://html5demos.com/video-canvas http://browsertests.herokuapp.com/media/mdn_samples_webgl_sample8/index.html https://codereview.chromium.org/445013002/diff/740001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:103: desc.fFlags = kRenderTarget_GrTextureFlagBit | kNoStencil_GrTextureFlagBit; On 2014/12/18 23:12:31, sievers wrote: > while you are in here: mind putting a comment that 'kRenderTarget' here avoids a > copy before readback in skia (as I recently found out)? I assume that's why it's > used here, and it's not really obvious why we would need this flag since we > don't render to the texture, but only read it back. I didn't consider about kRenderTarget because I blindly copy it :) I added the comment. Done.
Hi @ben, @aa, could you review mojo/services/html_viewer/webmediaplayer_factory.cc ?
mojo/ lgtm
On 2015/01/02 19:52:24, jamesr wrote: > mojo/ lgtm Thank you for review!
The CQ bit was checked by dongseong.hwang@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/445013002/780001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2015/01/03 16:56:01, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) @scherkus, media/DEPS seems to need lgtm again for some reasons. Could you lgtm again? Missing LGTM from OWNERS of dependencies added to DEPS: '+skia/ext',
The CQ bit was checked by dongseong.hwang@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/445013002/780001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
dongseong.hwang@intel.com changed reviewers: + xhwang@chromium.org
On 2015/01/04 12:05:47, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) @xhwang, could you review media/DEPS because @scherkus seems to be OOO? @scherkus already reviewed all media/ but commit-bot complain media/DEPS for some reasons.
On 2015/01/08 16:04:57, dshwang wrote: > On 2015/01/04 12:05:47, I haz the power (commit-bot) wrote: > > Try jobs failed on following builders: > > chromium_presubmit on tryserver.chromium.linux > > > (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > @xhwang, could you review media/DEPS because @scherkus seems to be OOO? > @scherkus already reviewed all media/ but commit-bot complain media/DEPS for > some reasons. You need a skia/ OWNER to review your DEPS change because you added dependencies on '+skia/ext'.
On 2015/01/08 16:59:21, xhwang wrote: > On 2015/01/08 16:04:57, dshwang wrote: > > On 2015/01/04 12:05:47, I haz the power (commit-bot) wrote: > > > Try jobs failed on following builders: > > > chromium_presubmit on tryserver.chromium.linux > > > > > > (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > > > @xhwang, could you review media/DEPS because @scherkus seems to be OOO? > > @scherkus already reviewed all media/ but commit-bot complain media/DEPS for > > some reasons. > > You need a skia/ OWNER to review your DEPS change because you added dependencies > on '+skia/ext'. Thank you for explaining. It's impressed for presubmit to check cross. @junov, could you review media/DEPS as skia/ owner? https://codereview.chromium.org/445013002/diff/780001/media/filters/skcanvas_... File media/filters/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/445013002/diff/780001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:240: skia::RefPtr<GrTexture> texture = skia::AdoptRef( I use skia/ext/refptr.h here. If I use skia internal refptr, presubmit complains that I must use skia/ext/refptr.h
lgtm (this DEPS ownership rule thing is weird)
The CQ bit was checked by dongseong.hwang@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/445013002/780001
On 2015/01/08 19:53:30, junov wrote: > lgtm > > (this DEPS ownership rule thing is weird) indeed. thx for reviewing :)
Message was sent while issue was closed.
Committed patchset #20 (id:780001)
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/0c4e9d8781aea6e52fdb4a7aee978817910c67ea Cr-Commit-Position: refs/heads/master@{#310577}
Message was sent while issue was closed.
A revert of this CL (patchset #20 id:780001) has been created in https://codereview.chromium.org/818853004/ by lizeb@chromium.org. The reason for reverting is: This change breaks android browser_content_tests. It was only catched by downstream bots. Reverting for now. .
Message was sent while issue was closed.
perkj@chromium.org changed reviewers: + perkj@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/445013002/diff/780001/media/filters/skcanvas_... File media/filters/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/445013002/diff/780001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:432: DCHECK(!accelerated_generator_); This DCHECK hit on Nexus 5 in an android content browsertest. WebRtcBrowserTest.CallAndVerifyVideoMutingWorks http://build.chromium.org/p/chromium.webrtc/builders/Android%20Tests%20%28dbg... Removing this DCHECK pass the test.
Message was sent while issue was closed.
https://codereview.chromium.org/445013002/diff/780001/media/filters/skcanvas_... File media/filters/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/445013002/diff/780001/media/filters/skcanvas_... media/filters/skcanvas_video_renderer.cc:432: DCHECK(!accelerated_generator_); On 2015/01/12 12:55:26, perkj wrote: > This DCHECK hit on Nexus 5 in an android content browsertest. > WebRtcBrowserTest.CallAndVerifyVideoMutingWorks Thank you for reporting. Did you test using ToT? I'll check it again. > > http://build.chromium.org/p/chromium.webrtc/builders/Android%20Tests%20%28dbg... > > Removing this DCHECK pass the test. I believe DCHECK(context_3d.gl) is hit instead of DCHECK(!accelerated_generator_) and I fixed it recently in https://codereview.chromium.org/812613003/
Message was sent while issue was closed.
Yes- I tested using the latest code on Nexus 5. The bots also runs latest and greatest. Thanks for looking. On Mon, Jan 12, 2015 at 2:04 PM, <dongseong.hwang@intel.com> wrote: > > https://codereview.chromium.org/445013002/diff/780001/ > media/filters/skcanvas_video_renderer.cc > File media/filters/skcanvas_video_renderer.cc (right): > > https://codereview.chromium.org/445013002/diff/780001/ > media/filters/skcanvas_video_renderer.cc#newcode432 > media/filters/skcanvas_video_renderer.cc:432: > DCHECK(!accelerated_generator_); > On 2015/01/12 12:55:26, perkj wrote: > >> This DCHECK hit on Nexus 5 in an android content browsertest. >> WebRtcBrowserTest.CallAndVerifyVideoMutingWorks >> > > Thank you for reporting. Did you test using ToT? I'll check it again. > > > > http://build.chromium.org/p/chromium.webrtc/builders/ > Android%20Tests%20%28dbg%29%20%28KK%20Nexus5%29/builds/ > 12407/steps/content_browsertests/logs/stdio > > Removing this DCHECK pass the test. >> > > I believe DCHECK(context_3d.gl) is hit instead of > DCHECK(!accelerated_generator_) > and I fixed it recently in https://codereview.chromium.org/812613003/ > > https://codereview.chromium.org/445013002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2015/01/12 13:08:17, perkj wrote: > Yes- I tested using the latest code on Nexus 5. The bots also runs latest > and greatest. > Thanks for looking. > > On Mon, Jan 12, 2015 at 2:04 PM, <mailto:dongseong.hwang@intel.com> wrote: > > > > > https://codereview.chromium.org/445013002/diff/780001/ > > media/filters/skcanvas_video_renderer.cc > > File media/filters/skcanvas_video_renderer.cc (right): > > > > https://codereview.chromium.org/445013002/diff/780001/ > > media/filters/skcanvas_video_renderer.cc#newcode432 > > media/filters/skcanvas_video_renderer.cc:432: > > DCHECK(!accelerated_generator_); > > On 2015/01/12 12:55:26, perkj wrote: > > > >> This DCHECK hit on Nexus 5 in an android content browsertest. > >> WebRtcBrowserTest.CallAndVerifyVideoMutingWorks > >> > > > > Thank you for reporting. Did you test using ToT? I'll check it again. > > > > > > > > http://build.chromium.org/p/chromium.webrtc/builders/ > > Android%20Tests%20%28dbg%29%20%28KK%20Nexus5%29/builds/ > > 12407/steps/content_browsertests/logs/stdio > > > > Removing this DCHECK pass the test. > >> > > > > I believe DCHECK(context_3d.gl) is hit instead of > > DCHECK(!accelerated_generator_) > > and I fixed it recently in https://codereview.chromium.org/812613003/ > > > > https://codereview.chromium.org/445013002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Oh, sorry for inconvenience. I checked few WebRTCBrowserTests in crrev.com/812613003 but there seems to be crash still for other reasons. I'll fix it asap. (in ~2 hours)
Message was sent while issue was closed.
Awsome! Thanks, I will leave it as it is then. FYI, the test pass if I just remove the DCHECK. On Mon, Jan 12, 2015 at 2:16 PM, <dongseong.hwang@intel.com> wrote: > On 2015/01/12 13:08:17, perkj wrote: > >> Yes- I tested using the latest code on Nexus 5. The bots also runs latest >> and greatest. >> Thanks for looking. >> > > On Mon, Jan 12, 2015 at 2:04 PM, <mailto:dongseong.hwang@intel.com> >> wrote: >> > > > >> > https://codereview.chromium.org/445013002/diff/780001/ >> > media/filters/skcanvas_video_renderer.cc >> > File media/filters/skcanvas_video_renderer.cc (right): >> > >> > https://codereview.chromium.org/445013002/diff/780001/ >> > media/filters/skcanvas_video_renderer.cc#newcode432 >> > media/filters/skcanvas_video_renderer.cc:432: >> > DCHECK(!accelerated_generator_); >> > On 2015/01/12 12:55:26, perkj wrote: >> > >> >> This DCHECK hit on Nexus 5 in an android content browsertest. >> >> WebRtcBrowserTest.CallAndVerifyVideoMutingWorks >> >> >> > >> > Thank you for reporting. Did you test using ToT? I'll check it again. >> > >> > >> > >> > http://build.chromium.org/p/chromium.webrtc/builders/ >> > Android%20Tests%20%28dbg%29%20%28KK%20Nexus5%29/builds/ >> > 12407/steps/content_browsertests/logs/stdio >> > >> > Removing this DCHECK pass the test. >> >> >> > >> > I believe DCHECK(context_3d.gl) is hit instead of >> > DCHECK(!accelerated_generator_) >> > and I fixed it recently in https://codereview.chromium.org/812613003/ >> > >> > https://codereview.chromium.org/445013002/ >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > Oh, sorry for inconvenience. I checked few WebRTCBrowserTests in > crrev.com/812613003 but there seems to be crash still for other reasons. > I'll fix it asap. (in ~2 hours) > > https://codereview.chromium.org/445013002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2015/01/12 13:21:46, perkj wrote: > Awsome! Thanks, I will leave it as it is then. FYI, the test pass if I just > remove the DCHECK. I'm fixing it in https://codereview.chromium.org/848683003/ |
