Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1133563010:
Add a GpuMemoryBuffer pool that creates hardware backed VideoFrames. (Closed)
|
Created:
5 years, 7 months ago by Daniele Castagna Modified:
5 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a GpuMemoryBuffer pool that creates hardware backed VideoFrames.
A new pool of resources -consisting of GpuMemoryBuffers, textures and
images- has been added to media/video and an instance is now allocated
in VideoRendererImpl.
This pool, disabled in this cl, will allow VideoRenderImpl to convert
software VideoFrames into VideoFrames backed by native resources.
Once we enable this conversion the software video playback will
benefit from this in different ways:
- Reducing the number of copies of VideoFrame buffers, potentially
having only one when native GpuMemoryBuffers are available.
- Moving one copy in the media thread and removing it from the critical
path.
BUG=485859
Committed: https://crrev.com/612cc633d1e789d92208bdacdfa22eac5a4c800a
Cr-Commit-Position: refs/heads/master@{#330053}
Patch Set 1 : #
Total comments: 76
Patch Set 2 : Partially address reviews' comments. Add tests. #Patch Set 3 : Avoid giving resources' ownerships to callbacks. Mojo something now compiles. #Patch Set 4 : Fix a mem leak. Remove TODO in VideoRendererImpl. #
Total comments: 38
Patch Set 5 : Address dalecurtis' comments. #
Total comments: 4
Patch Set 6 : s/IsFrameResources/AreFrameResources/. Fix a typo in a comment. #Patch Set 7 : IsTextureRGSupported. #Patch Set 8 : Rebase on master. #
Total comments: 6
Patch Set 9 : Address reveman@'s nits. #Messages
Total messages: 24 (5 generated)
dcastagna@chromium.org changed reviewers: + reveman@chromium.org
Patchset #1 (id:1) has been deleted
https://codereview.chromium.org/1133563010/diff/20001/content/renderer/media/... File content/renderer/media/renderer_gpu_video_accelerator_factories.h (right): https://codereview.chromium.org/1133563010/diff/20001/content/renderer/media/... content/renderer/media/renderer_gpu_video_accelerator_factories.h:72: // thread associated with the 'task_runner' passed to the constructor. nit: think this is implied from the class description above https://codereview.chromium.org/1133563010/diff/20001/content/renderer/media/... content/renderer/media/renderer_gpu_video_accelerator_factories.h:103: gpu::GpuMemoryBufferManager* gpu_memory_buffer_manager_; nit: const pointer https://codereview.chromium.org/1133563010/diff/20001/media/renderers/mock_gp... File media/renderers/mock_gpu_video_accelerator_factories.cc (right): https://codereview.chromium.org/1133563010/diff/20001/media/renderers/mock_gp... media/renderers/mock_gpu_video_accelerator_factories.cc:18: DCHECK_EQ(gfx::GpuMemoryBuffer::R_8, format); nit: move this to AllocateGpuMemoryBuffer and remove |format_| from this class. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:75: VideoFrame::Format); nit: missing param name https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:90: scoped_refptr<GpuVideoAcceleratorFactories> gpu_factories_, nit: remove '_' suffix. and again, dunno about media/ but we'd typically use a raw pointer here in chromium https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:124: DCHECK_LE(bytes_per_row, static_cast<int>(dest_stride)); what it dest_stride is negative? https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:125: DCHECK_LE(bytes_per_row, source_stride); can source_stride be negative? https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:148: gpu_factories_ ? gpu_factories_->GetGLES2Interface() : nullptr; Do both of these need to be allowed to be null? Maybe improve readability a bit with: if (!gpu_factories_) return video_frame; gpu::gles2::GLES2Interface* gles2 = gpu_factories_->GetGLES2Interface(); if (!gles2) return video_frame; https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:172: const gfx::Size size(width, height); creating this seems useless and just confusing as it shadows |size| above. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:175: size.height(), GL_R8_EXT); Do we need to check that GL_R8_EXT is supported? https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:195: base::Passed(&frame_resources)), Not sure passing frame resources containing GMBs and context3d resources in a callback is a good idea. I understand that our lost context logic in media/ is not the best but I don't think we should rely on that here. You need to be able to delete these resources in a lost context situation and that's not possible when the ownership is passed to a callback. Even if we ignore the lost context situation, proper shutdown requires use to delete these resources before the GMB-manager and context3d. What if the media/ thread is torn down before the compositor thread but the compositor thread has a reference to this callback? I think it would be better to always keep ownership of these resources here in the pool and remove the ref-counting. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:204: auto resources_it = resources_pool_.begin(); ? https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:206: auto frame_resources_linked_ptr = resources_pool_.front(); scoped_ptr<FrameResources> frame_resources = make_scoped_ptr(resources_pool_.front().release())? or see comment below https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:212: base::Passed(&frame_resources))); pass frame_resource as linked_ptr and get rid of the scoped_ptr mess.. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:225: auto frame_resources_linked_ptr = resources_pool_.front(); make consistent with above code https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:268: DCHECK(frame_resources); nit: why check frame_resources here? it seems a bit overkill to have a private method where first thing we do is check that one of the params is non-null. if there's a chance that this can be null, then I'd move that check closer to the source (e.g. when you construct the callback) as it's not going be be nearly as useful here. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:272: for (size_t i = 0; i < arraysize(frame_resources->plane_resources); ++i) { for (PlaneResource& plane_resource : frame_resources->plane_resources) ? https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:301: linked_ptr<FrameResources>(frame_resources.release())); make_linked_ptr? https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool.h (right): https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.h:28: const scoped_refptr<GpuVideoAcceleratorFactories>& gpu_factories); I dunno about media put in other parts of chromium we typically always use a raw pointer when passing a ref-counted instance to a function https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.h:31: const scoped_refptr<VideoFrame> MaybeCreateHardwareFrame( why 'const' return type? https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.h:32: const scoped_refptr<VideoFrame>); nit: missing param name
dcastagna@chromium.org changed reviewers: + dalecurtis@chromium.org
+dalecurtis, PTAL.
Just high level and nits I've noticed, didn't fully review details of the pool https://codereview.chromium.org/1133563010/diff/20001/media/renderers/video_r... File media/renderers/video_renderer_impl.cc (right): https://codereview.chromium.org/1133563010/diff/20001/media/renderers/video_r... media/renderers/video_renderer_impl.cc:507: // TODO(dcastagna): Enable GpuMemoryBuffer VideoFrames. Some notes: - To avoid blocking the media thread we might want to consider posting to the worker pool for these copies. - In some cases the VideoRendererAlgorithm knows which frames we will always drop, so say at 16x playback rate me may want to skip the uploading for frames we know we'll never display. - In the above case the code will let the frame pool grow to 12 frames, what happens in OOM situations here? https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:51: unsigned texture_id = 0u; Use the type size, uint32_t or whatever. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:90: scoped_refptr<GpuVideoAcceleratorFactories> gpu_factories_, const& https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:97: std::deque<linked_ptr<FrameResources>> resources_pool_; Can you avoid using linked_ptr here? It makes things harder to reason about. If not it's fine, but I'm always sad to see it :) https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:111: const int source_stride, Remove const for primitive types that aren't pointers. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:119: CHECK(buffer->Map(&data)); Seems we are more likely to OOM GPU buffers, should the frame be gracefully returned instead here? https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:135: // Creates a VideoFrame backed by native textures starting from a software Comments should go with the function definition not the implementation. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:334: case media::VideoFrame::NATIVE_TEXTURE: I don't think this should have a NOTREACHED(), it should gracefully return the frame no? https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool.h (right): https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.h:23: // VideoFrame the object passed as parameter will be returned. Needs a lot more details about OOM situations and some minor details about how recycling is implemented. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.h:28: const scoped_refptr<GpuVideoAcceleratorFactories>& gpu_factories); On 2015/05/12 03:20:26, reveman wrote: > I dunno about media put in other parts of chromium we typically always use a raw > pointer when passing a ref-counted instance to a function media passes these via const& which should do the same thing and avoid accidentally losing some refs. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.h:32: const scoped_refptr<VideoFrame>); On 2015/05/12 03:20:25, reveman wrote: > nit: missing param name Should be const&
Also I was promised a unit test via chat :)
https://codereview.chromium.org/1133563010/diff/20001/content/renderer/media/... File content/renderer/media/renderer_gpu_video_accelerator_factories.h (right): https://codereview.chromium.org/1133563010/diff/20001/content/renderer/media/... content/renderer/media/renderer_gpu_video_accelerator_factories.h:72: // thread associated with the 'task_runner' passed to the constructor. On 2015/05/12 at 03:20:24, reveman wrote: > nit: think this is implied from the class description above Removed the comment. https://codereview.chromium.org/1133563010/diff/20001/content/renderer/media/... content/renderer/media/renderer_gpu_video_accelerator_factories.h:103: gpu::GpuMemoryBufferManager* gpu_memory_buffer_manager_; On 2015/05/12 at 03:20:24, reveman wrote: > nit: const pointer Do you mean gpu::GpuMemoryBufferManager* const? Done. const gpu::GpuMemoryBufferManager* can't be done since we use AllocateGpuMemoryBuffer that is not const. https://codereview.chromium.org/1133563010/diff/20001/media/renderers/mock_gp... File media/renderers/mock_gpu_video_accelerator_factories.cc (right): https://codereview.chromium.org/1133563010/diff/20001/media/renderers/mock_gp... media/renderers/mock_gpu_video_accelerator_factories.cc:18: DCHECK_EQ(gfx::GpuMemoryBuffer::R_8, format); On 2015/05/12 at 03:20:24, reveman wrote: > nit: move this to AllocateGpuMemoryBuffer and remove |format_| from this class. Done. https://codereview.chromium.org/1133563010/diff/20001/media/renderers/video_r... File media/renderers/video_renderer_impl.cc (right): https://codereview.chromium.org/1133563010/diff/20001/media/renderers/video_r... media/renderers/video_renderer_impl.cc:507: // TODO(dcastagna): Enable GpuMemoryBuffer VideoFrames. On 2015/05/12 at 17:23:20, DaleCurtis wrote: > Some notes: > - To avoid blocking the media thread we might want to consider posting to the worker pool for these copies. Good point. In our tests we noticed that the copy here blocks the media thread. Overall the playback is still faster. Should we leave the copy in the worker pool as a later optimization? > - In some cases the VideoRendererAlgorithm knows which frames we will always drop, so say at 16x playback rate me may want to skip the uploading for frames we know we'll never display. It'd be nice to skip the copy if the frame is going to be dropped. Do you have in mind a better place, after VideoRendererAlgorithm determined that we're not dropping the frame, where we could do the copy? > - In the above case the code will let the frame pool grow to 12 frames, what happens in OOM situations here? If the original software frames are not needed for video decoding anymore, the software frames will be dropped once we create the hardware frames, so the total memory usage should be the same. About this, we were planning to implement MemoryDumpProvider for the pool (the ones in VpxVideoDecoder too) so we can better trace what's going on with memory and pools. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:51: unsigned texture_id = 0u; On 2015/05/12 at 17:23:20, DaleCurtis wrote: > Use the type size, uint32_t or whatever. In cc unsigned is used for resouces/textures/images ids. Do you prefer a sized type? https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:75: VideoFrame::Format); On 2015/05/12 at 03:20:25, reveman wrote: > nit: missing param name Done. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:90: scoped_refptr<GpuVideoAcceleratorFactories> gpu_factories_, On 2015/05/12 at 03:20:25, reveman wrote: > nit: remove '_' suffix. and again, dunno about media/ but we'd typically use a raw pointer here in chromium _ removed. This is used as a callback, the scoped_refptr here make it so the callback keeps the gpu_factories alive. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:90: scoped_refptr<GpuVideoAcceleratorFactories> gpu_factories_, On 2015/05/12 at 17:23:20, DaleCurtis wrote: > const& Done. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:97: std::deque<linked_ptr<FrameResources>> resources_pool_; On 2015/05/12 at 17:23:20, DaleCurtis wrote: > Can you avoid using linked_ptr here? It makes things harder to reason about. If not it's fine, but I'm always sad to see it :) I was using a ScopedVector but they suggested me to use linked_ptr so that I can switch to a deque and make it clearer how we're using the pool. I'll follow up with another iteration where the pool keeps the ownership if FrameResources all the time. That should make it easier to avoid the linked_ptr. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:111: const int source_stride, On 2015/05/12 at 17:23:20, DaleCurtis wrote: > Remove const for primitive types that aren't pointers. Done. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:119: CHECK(buffer->Map(&data)); On 2015/05/12 at 17:23:20, DaleCurtis wrote: > Seems we are more likely to OOM GPU buffers, should the frame be gracefully returned instead here? AFAIU Map doesn't fail with the current GpuMemoryBuffer implementations we have right now. The memory buffer allocations can fail, we can return the frame there if that happens. reveman: what do you think about this? https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:124: DCHECK_LE(bytes_per_row, static_cast<int>(dest_stride)); On 2015/05/12 at 03:20:25, reveman wrote: > what it dest_stride is negative? Changed the DCHECK. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:125: DCHECK_LE(bytes_per_row, source_stride); On 2015/05/12 at 03:20:25, reveman wrote: > can source_stride be negative? It can't, software videoframes always have positive strides. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:135: // Creates a VideoFrame backed by native textures starting from a software On 2015/05/12 at 17:23:20, DaleCurtis wrote: > Comments should go with the function definition not the implementation. I'm OK moving it, but https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Com... seems to disagree with your statement. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:148: gpu_factories_ ? gpu_factories_->GetGLES2Interface() : nullptr; On 2015/05/12 at 03:20:25, reveman wrote: > Do both of these need to be allowed to be null? Maybe improve readability a bit with: > > if (!gpu_factories_) > return video_frame; > > gpu::gles2::GLES2Interface* gles2 = gpu_factories_->GetGLES2Interface(); > if (!gles2) > return video_frame; gpu_factories_ can be null, for example when #disable-accelerated-video-decode is true. gles2 can be null when a context can't be created (look at RendererGpuVideoAcceleratorFactories::GetContext3d) Changed it as you suggested. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:172: const gfx::Size size(width, height); On 2015/05/12 at 03:20:25, reveman wrote: > creating this seems useless and just confusing as it shadows |size| above. Done. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:175: size.height(), GL_R8_EXT); On 2015/05/12 at 03:20:25, reveman wrote: > Do we need to check that GL_R8_EXT is supported? Yes we do. I can follow up with another cl since this is not a real issue until we enable the pool. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:195: base::Passed(&frame_resources)), On 2015/05/12 at 03:20:25, reveman wrote: > Not sure passing frame resources containing GMBs and context3d resources in a callback is a good idea. I understand that our lost context logic in media/ is not the best but I don't think we should rely on that here. You need to be able to delete these resources in a lost context situation and that's not possible when the ownership is passed to a callback. Even if we ignore the lost context situation, proper shutdown requires use to delete these resources before the GMB-manager and context3d. What if the media/ thread is torn down before the compositor thread but the compositor thread has a reference to this callback? > > I think it would be better to always keep ownership of these resources here in the pool and remove the ref-counting. I'll follow up with another iteration on this CL keeping FrameResource ownerships inside the pool all the time. We'll still need the ref-counting since we need to wait for all the VideoFrame sent to the compositor to come back before we can destroy the pool. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:204: auto resources_it = resources_pool_.begin(); On 2015/05/12 at 03:20:25, reveman wrote: > ? Done. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:206: auto frame_resources_linked_ptr = resources_pool_.front(); On 2015/05/12 at 03:20:25, reveman wrote: > scoped_ptr<FrameResources> frame_resources = make_scoped_ptr(resources_pool_.front().release())? > > or see comment below Done. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:212: base::Passed(&frame_resources))); On 2015/05/12 at 03:20:25, reveman wrote: > pass frame_resource as linked_ptr and get rid of the scoped_ptr mess.. Done as above. I preferred to use scoped_ptr as soon as possible. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:225: auto frame_resources_linked_ptr = resources_pool_.front(); On 2015/05/12 at 03:20:25, reveman wrote: > make consistent with above code Done. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:268: DCHECK(frame_resources); On 2015/05/12 at 03:20:24, reveman wrote: > nit: why check frame_resources here? it seems a bit overkill to have a private method where first thing we do is check that one of the params is non-null. if there's a chance that this can be null, then I'd move that check closer to the source (e.g. when you construct the callback) as it's not going be be nearly as useful here. Done. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:272: for (size_t i = 0; i < arraysize(frame_resources->plane_resources); ++i) { On 2015/05/12 at 03:20:25, reveman wrote: > for (PlaneResource& plane_resource : frame_resources->plane_resources) ? Done. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:301: linked_ptr<FrameResources>(frame_resources.release())); On 2015/05/12 at 03:20:24, reveman wrote: > make_linked_ptr? Done. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:334: case media::VideoFrame::NATIVE_TEXTURE: On 2015/05/12 at 17:23:20, DaleCurtis wrote: > I don't think this should have a NOTREACHED(), it should gracefully return the frame no? These formats are not supported by VideoResourceUpdater::VerifyFrame and it would fail there. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool.h (right): https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.h:23: // VideoFrame the object passed as parameter will be returned. On 2015/05/12 at 17:23:20, DaleCurtis wrote: > Needs a lot more details about OOM situations and some minor details about how recycling is implemented. Shouldn't the comment on the interface be only high level view on what it does and how to use it? More details on how recycling is implemented are in the .cc file. Why are you so concerned about OOMing? Don't we just fail everywhere else in Chromium if we OOM? The renderer process is probably going to crash before the failed allocation reaches the pool. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.h:28: const scoped_refptr<GpuVideoAcceleratorFactories>& gpu_factories); On 2015/05/12 at 17:23:20, DaleCurtis wrote: > On 2015/05/12 03:20:26, reveman wrote: > > I dunno about media put in other parts of chromium we typically always use a raw > > pointer when passing a ref-counted instance to a function > > media passes these via const& which should do the same thing and avoid accidentally losing some refs. Leaving the const& as suggested by dalecurtis. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.h:31: const scoped_refptr<VideoFrame> MaybeCreateHardwareFrame( On 2015/05/12 at 03:20:25, reveman wrote: > why 'const' return type? Because if we can't create the hardwareframe we just retun the param, that is const, so the return value needs to be const as well. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.h:32: const scoped_refptr<VideoFrame>); On 2015/05/12 at 03:20:25, reveman wrote: > nit: missing param name Done. Also changed it to &. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.h:32: const scoped_refptr<VideoFrame>); On 2015/05/12 at 17:23:20, DaleCurtis wrote: > On 2015/05/12 03:20:25, reveman wrote: > > nit: missing param name > > Should be const& Done.
PTAL. The FrameResources ownerships is not passed around to callbacks. Fixed a compilation error in mojo_renderer_service.cc. I'm not sure if we should pass a gpu_accelerator_factory to VideoRendererImpl there too.
Didn't have time to review everything else tonight. https://codereview.chromium.org/1133563010/diff/20001/media/renderers/video_r... File media/renderers/video_renderer_impl.cc (right): https://codereview.chromium.org/1133563010/diff/20001/media/renderers/video_r... media/renderers/video_renderer_impl.cc:507: // TODO(dcastagna): Enable GpuMemoryBuffer VideoFrames. On 2015/05/12 21:21:29, Daniele Castagna wrote: > On 2015/05/12 at 17:23:20, DaleCurtis wrote: > > Some notes: > > - To avoid blocking the media thread we might want to consider posting to the > worker pool for these copies. > > Good point. In our tests we noticed that the copy here blocks the media thread. > Overall the playback is still faster. Should we leave the copy in the worker > pool as a later optimization? Should be okay to do in a second pass, we can watch the telemetry scores to see if it's necessary. To implement we could either refactor FrameReady slightly so that the queuing portion can be posted to the worker pool. Or, we could have GpuMemoryBufferPool immediately return a shell frame and have the copy happen in the background on the worker pool thread. Though we'd need to make sure the copy completes before the frame is actually displayed. > > > - In some cases the VideoRendererAlgorithm knows which frames we will always > drop, so say at 16x playback rate me may want to skip the uploading for frames > we know we'll never display. > > It'd be nice to skip the copy if the frame is going to be dropped. Do you have > in mind a better place, after VideoRendererAlgorithm determined that we're not > dropping the frame, where we could do the copy? Well you'd need to integrate this code with the Algorithm instead somehow or expose some information from the EnqueueFrame() call in AddReadyFrame_Locked() which would indicate if it expects to use the frame. There are reasons the frame may still be used though, which would mean a copy happening on the compositor thread sometimes. > > > - In the above case the code will let the frame pool grow to 12 frames, what > happens in OOM situations here? > > If the original software frames are not needed for video decoding anymore, the > software frames will be dropped once we create the hardware frames, so the total > memory usage should be the same. > About this, we were planning to implement MemoryDumpProvider for the pool (the > ones in VpxVideoDecoder too) so we can better trace what's going on with memory > and pools. Okay this was my confusion. I thought GPU memory might be more limited than system memory, and thus fail in ways that we need to gracefully handle. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:51: unsigned texture_id = 0u; On 2015/05/12 21:21:30, Daniele Castagna wrote: > On 2015/05/12 at 17:23:20, DaleCurtis wrote: > > Use the type size, uint32_t or whatever. > > In cc unsigned is used for resouces/textures/images ids. Do you prefer a sized > type? We don't use 'unsigned' in media/ code, but if this matches the cc code that's okay with me. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:135: // Creates a VideoFrame backed by native textures starting from a software On 2015/05/12 21:21:30, Daniele Castagna wrote: > On 2015/05/12 at 17:23:20, DaleCurtis wrote: > > Comments should go with the function definition not the implementation. > > I'm OK moving it, but > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Com... > seems to disagree with your statement. That seems to agree with me? Can you highlight the text that disagrees, I'm not seeing it :) https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool.h (right): https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.h:31: const scoped_refptr<VideoFrame> MaybeCreateHardwareFrame( On 2015/05/12 21:21:30, Daniele Castagna wrote: > On 2015/05/12 at 03:20:25, reveman wrote: > > why 'const' return type? > > Because if we can't create the hardwareframe we just retun the param, that is > const, so the return value needs to be const as well. Is this still true after using const& it should just make a copy or something?
Thank you Dale for your first round of comments! Looking forward for the rest of the review. https://codereview.chromium.org/1133563010/diff/20001/media/renderers/video_r... File media/renderers/video_renderer_impl.cc (right): https://codereview.chromium.org/1133563010/diff/20001/media/renderers/video_r... media/renderers/video_renderer_impl.cc:507: // TODO(dcastagna): Enable GpuMemoryBuffer VideoFrames. On 2015/05/13 at 02:52:14, DaleCurtis wrote: > On 2015/05/12 21:21:29, Daniele Castagna wrote: > > On 2015/05/12 at 17:23:20, DaleCurtis wrote: > > > Some notes: > > > - To avoid blocking the media thread we might want to consider posting to the > > worker pool for these copies. > > > > Good point. In our tests we noticed that the copy here blocks the media thread. > > Overall the playback is still faster. Should we leave the copy in the worker > > pool as a later optimization? > > Should be okay to do in a second pass, we can watch the telemetry scores to see if it's necessary. To implement we could either refactor FrameReady slightly so that the queuing portion can be posted to the worker pool. Or, we could have GpuMemoryBufferPool immediately return a shell frame and have the copy happen in the background on the worker pool thread. Though we'd need to make sure the copy completes before the frame is actually displayed. Sounds good to me to do it in a second pass. I'm going to delete these two lines since we need another cl in cc before we can enable this. I put the commented lines here here to start this kind of discussion. > > > > > > - In some cases the VideoRendererAlgorithm knows which frames we will always > > drop, so say at 16x playback rate me may want to skip the uploading for frames > > we know we'll never display. > > > > It'd be nice to skip the copy if the frame is going to be dropped. Do you have > > in mind a better place, after VideoRendererAlgorithm determined that we're not > > dropping the frame, where we could do the copy? > > Well you'd need to integrate this code with the Algorithm instead somehow or expose some information from the EnqueueFrame() call in AddReadyFrame_Locked() which would indicate if it expects to use the frame. There are reasons the frame may still be used though, which would mean a copy happening on the compositor thread sometimes. I guess this would end up in the second pass too, right? > > > > > > - In the above case the code will let the frame pool grow to 12 frames, what > > happens in OOM situations here? > > > > If the original software frames are not needed for video decoding anymore, the > > software frames will be dropped once we create the hardware frames, so the total > > memory usage should be the same. > > About this, we were planning to implement MemoryDumpProvider for the pool (the > > ones in VpxVideoDecoder too) so we can better trace what's going on with memory > > and pools. > > Okay this was my confusion. I thought GPU memory might be more limited than system memory, and thus fail in ways that we need to gracefully handle. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:51: unsigned texture_id = 0u; On 2015/05/13 at 02:52:14, DaleCurtis wrote: > On 2015/05/12 21:21:30, Daniele Castagna wrote: > > On 2015/05/12 at 17:23:20, DaleCurtis wrote: > > > Use the type size, uint32_t or whatever. > > > > In cc unsigned is used for resouces/textures/images ids. Do you prefer a sized > > type? > > We don't use 'unsigned' in media/ code, but if this matches the cc code that's okay with me. It's your call. If you prefer a signed type I'll change it. If you don't say anything I'm going to leave it like this. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:135: // Creates a VideoFrame backed by native textures starting from a software On 2015/05/13 at 02:52:14, DaleCurtis wrote: > On 2015/05/12 21:21:30, Daniele Castagna wrote: > > On 2015/05/12 at 17:23:20, DaleCurtis wrote: > > > Comments should go with the function definition not the implementation. > > > > I'm OK moving it, but > > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Com... > > seems to disagree with your statement. > > That seems to agree with me? Can you highlight the text that disagrees, I'm not seeing it :) "Declaration comments describe use of the function; comments at the definition of a function describe operation." Here we are defining a method, so I thought I'd be a little bit more verbose about the internals of the method. You said: "Comments should go with the function definition not the implementation.". Assuming you meant declaration instead of definition, I think this comment makes sense on top of the implementation since declaration comment should describe only the use of the function. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool.h (right): https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.h:31: const scoped_refptr<VideoFrame> MaybeCreateHardwareFrame( On 2015/05/13 at 02:52:14, DaleCurtis wrote: > On 2015/05/12 21:21:30, Daniele Castagna wrote: > > On 2015/05/12 at 03:20:25, reveman wrote: > > > why 'const' return type? > > > > Because if we can't create the hardwareframe we just retun the param, that is > > const, so the return value needs to be const as well. > > Is this still true after using const& it should just make a copy or something? No, changed it.
deferring to reveman@ for the pool impl review. I'm not familiar enough with the GPU side of things to asses correctness. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:51: unsigned texture_id = 0u; On 2015/05/13 04:29:45, Daniele Castagna wrote: > On 2015/05/13 at 02:52:14, DaleCurtis wrote: > > On 2015/05/12 21:21:30, Daniele Castagna wrote: > > > On 2015/05/12 at 17:23:20, DaleCurtis wrote: > > > > Use the type size, uint32_t or whatever. > > > > > > In cc unsigned is used for resouces/textures/images ids. Do you prefer a > sized > > > type? > > > > We don't use 'unsigned' in media/ code, but if this matches the cc code that's > okay with me. > > It's your call. If you prefer a signed type I'll change it. If you don't say > anything I'm going to leave it like this. I didn't mean use a signed type, I just meant give it an explicit size, like size_t or uint32_t, etc. If there are equivalent values for this code somewhere else using unsigned though, it's better to be consistent here (also in case this gets serialized via IPC). https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:135: // Creates a VideoFrame backed by native textures starting from a software On 2015/05/13 04:29:45, Daniele Castagna wrote: > On 2015/05/13 at 02:52:14, DaleCurtis wrote: > > On 2015/05/12 21:21:30, Daniele Castagna wrote: > > > On 2015/05/12 at 17:23:20, DaleCurtis wrote: > > > > Comments should go with the function definition not the implementation. > > > > > > I'm OK moving it, but > > > > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Com... > > > seems to disagree with your statement. > > > > That seems to agree with me? Can you highlight the text that disagrees, I'm > not seeing it :) > > "Declaration comments describe use of the function; comments at the definition > of a function describe operation." > > Here we are defining a method, so I thought I'd be a little bit more verbose > about the internals of the method. > > You said: "Comments should go with the function definition not the > implementation.". Assuming you meant declaration instead of definition, I think > this comment makes sense on top of the implementation since declaration comment > should describe only the use of the function. Hmm, I see; this is atypical for media/ style and I'd prefer if a meaty comment of this type was interleaved into the code below for better readability than a text blob here at the top. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:334: case media::VideoFrame::NATIVE_TEXTURE: On 2015/05/12 21:21:29, Daniele Castagna wrote: > On 2015/05/12 at 17:23:20, DaleCurtis wrote: > > I don't think this should have a NOTREACHED(), it should gracefully return the > frame no? > > These formats are not supported by VideoResourceUpdater::VerifyFrame and it > would fail there. What I mean is this method is definitely going to be called with these type of frames since VRI is always calling it, you need to not spam the logs when this happens :) https://codereview.chromium.org/1133563010/diff/80001/media/renderers/video_r... File media/renderers/video_renderer_impl.cc (right): https://codereview.chromium.org/1133563010/diff/80001/media/renderers/video_r... media/renderers/video_renderer_impl.cc:48: scoped_refptr<GpuVideoAcceleratorFactories> gpu_factories, const& ? https://codereview.chromium.org/1133563010/diff/80001/media/renderers/video_r... media/renderers/video_renderer_impl.cc:57: new GpuMemoryBufferVideoFramePool(task_runner, gpu_factories)), Do you want this owned by each VideoRendererImpl (one per video tag) or would it be better if the renderer owned a single pool that frames were served from? https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:29: // 'task_runner' is associated to the thread where the context of Style is || instead of '' for designating variables. https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:40: const scoped_refptr<VideoFrame> CreateHardwareFrame( Remove const https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:41: scoped_refptr<VideoFrame> video_frame); const& https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:110: // bytes_per_row is expected to be less or equal than the strides of the two bytes_per_row and all other variables in comments, should have || around them. Please find all instances in this file and correct. https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:139: // Creates a VideoFrame backed by native textures starting from a software Again, strongly prefer if the comment was reflowed into the function body. https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:149: GpuMemoryBufferVideoFramePool::PoolImpl::CreateHardwareFrame( Fix const, const& stuff https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:218: GpuMemoryBufferVideoFramePool::PoolImpl::FrameResources* Why not scoped_ptr? https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:248: for (size_t i = 0; i < planes; ++i) { Don't need i it looks like, so why not for (const auto& plane_resource : frame_resources->plane_resources) https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:272: // TODO(dcastagna): As soon as the context lost is dealt with in media, Seems pretty important to fix sooner than later :) https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:301: auto it = std::find(resources_pool_.begin(), resources_pool_.end(), These will be released extremely frequently, so you'll want to do some profiling to ensure this find() isn't too expensive. https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:303: DCHECK(it != resources_pool_.end()); DCHECK_NE? https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:338: return video_frame; I'd just do nothing here and change the return at the end of the function to always return video_frame. https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:344: NOTREACHED(); Again remove this, these falls under the unsupported cases. https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool.h (right): https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.h:31: scoped_refptr<VideoFrame> MaybeCreateHardwareFrame( Method needs comment.
PTAL. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:51: unsigned texture_id = 0u; On 2015/05/14 at 00:58:57, DaleCurtis wrote: > On 2015/05/13 04:29:45, Daniele Castagna wrote: > > On 2015/05/13 at 02:52:14, DaleCurtis wrote: > > > On 2015/05/12 21:21:30, Daniele Castagna wrote: > > > > On 2015/05/12 at 17:23:20, DaleCurtis wrote: > > > > > Use the type size, uint32_t or whatever. > > > > > > > > In cc unsigned is used for resouces/textures/images ids. Do you prefer a > > sized > > > > type? > > > > > > We don't use 'unsigned' in media/ code, but if this matches the cc code that's > > okay with me. > > > > It's your call. If you prefer a signed type I'll change it. If you don't say > > anything I'm going to leave it like this. > > I didn't mean use a signed type, I just meant give it an explicit size, like size_t or uint32_t, etc. If there are equivalent values for this code somewhere else using unsigned though, it's better to be consistent here (also in case this gets serialized via IPC). Sorry, I meant sized not signed. Leaving it unsigned since that's what we have in cc. https://codereview.chromium.org/1133563010/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:135: // Creates a VideoFrame backed by native textures starting from a software On 2015/05/14 at 00:58:57, DaleCurtis wrote: > On 2015/05/13 04:29:45, Daniele Castagna wrote: > > On 2015/05/13 at 02:52:14, DaleCurtis wrote: > > > On 2015/05/12 21:21:30, Daniele Castagna wrote: > > > > On 2015/05/12 at 17:23:20, DaleCurtis wrote: > > > > > Comments should go with the function definition not the implementation. > > > > > > > > I'm OK moving it, but > > > > > > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Com... > > > > seems to disagree with your statement. > > > > > > That seems to agree with me? Can you highlight the text that disagrees, I'm > > not seeing it :) > > > > "Declaration comments describe use of the function; comments at the definition > > of a function describe operation." > > > > Here we are defining a method, so I thought I'd be a little bit more verbose > > about the internals of the method. > > > > You said: "Comments should go with the function definition not the > > implementation.". Assuming you meant declaration instead of definition, I think > > this comment makes sense on top of the implementation since declaration comment > > should describe only the use of the function. > > Hmm, I see; this is atypical for media/ style and I'd prefer if a meaty comment of this type was interleaved into the code below for better readability than a text blob here at the top. OK, better to be consistent with the rest of media. https://codereview.chromium.org/1133563010/diff/80001/media/renderers/video_r... File media/renderers/video_renderer_impl.cc (right): https://codereview.chromium.org/1133563010/diff/80001/media/renderers/video_r... media/renderers/video_renderer_impl.cc:48: scoped_refptr<GpuVideoAcceleratorFactories> gpu_factories, On 2015/05/14 at 00:58:57, DaleCurtis wrote: > const& ? Done. https://codereview.chromium.org/1133563010/diff/80001/media/renderers/video_r... media/renderers/video_renderer_impl.cc:57: new GpuMemoryBufferVideoFramePool(task_runner, gpu_factories)), On 2015/05/14 at 00:58:57, DaleCurtis wrote: > Do you want this owned by each VideoRendererImpl (one per video tag) or would it be better if the renderer owned a single pool that frames were served from? We were thinking about one per video tag right now. One per renderer would probably be better to limit the total memory used though. Should we leave the one pool for renderer in another iteration? https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:29: // 'task_runner' is associated to the thread where the context of On 2015/05/14 at 00:58:58, DaleCurtis wrote: > Style is || instead of '' for designating variables. Done. https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:40: const scoped_refptr<VideoFrame> CreateHardwareFrame( On 2015/05/14 at 00:58:57, DaleCurtis wrote: > Remove const Done. https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:41: scoped_refptr<VideoFrame> video_frame); On 2015/05/14 at 00:58:58, DaleCurtis wrote: > const& Done. https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:110: // bytes_per_row is expected to be less or equal than the strides of the two On 2015/05/14 at 00:58:57, DaleCurtis wrote: > bytes_per_row and all other variables in comments, should have || around them. Please find all instances in this file and correct. Done. https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:139: // Creates a VideoFrame backed by native textures starting from a software On 2015/05/14 at 00:58:58, DaleCurtis wrote: > Again, strongly prefer if the comment was reflowed into the function body. Done. https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:149: GpuMemoryBufferVideoFramePool::PoolImpl::CreateHardwareFrame( On 2015/05/14 at 00:58:57, DaleCurtis wrote: > Fix const, const& stuff Done. https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:218: GpuMemoryBufferVideoFramePool::PoolImpl::FrameResources* On 2015/05/14 at 00:58:58, DaleCurtis wrote: > Why not scoped_ptr? As per reveman's suggestion we decided to leave FrameResources ownership in the pool and just mark the object as in_use. Getting a FrameResouces gives you just a pointer to FrameResources and doesn't transfer the ownership. This will make it easier to account memory usage of the pool and to deal with context lost situations once we address that in media. https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:248: for (size_t i = 0; i < planes; ++i) { On 2015/05/14 at 00:58:57, DaleCurtis wrote: > Don't need i it looks like, so why not for (const auto& plane_resource : frame_resources->plane_resources) |planes| might be less than arraysize(frame_resources->plane_resources); https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:272: // TODO(dcastagna): As soon as the context lost is dealt with in media, On 2015/05/14 at 00:58:57, DaleCurtis wrote: > Seems pretty important to fix sooner than later :) Absolutely. I talked with posciak@ about this and discovered that a gpu process restart with hardware decoding would result in broken videos. :( I think we should deal with the context lost for media hardware decoding first, and then this pool could follow. https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:301: auto it = std::find(resources_pool_.begin(), resources_pool_.end(), On 2015/05/14 at 00:58:58, DaleCurtis wrote: > These will be released extremely frequently, so you'll want to do some profiling to ensure this find() isn't too expensive. I'll take a look at this before enabling the pool. resource_pool_ size is really little (size in the order of 10), it's really unlikely this is going to be an issue. https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:303: DCHECK(it != resources_pool_.end()); On 2015/05/14 at 00:58:57, DaleCurtis wrote: > DCHECK_NE? That doesn't work with iterators. :( https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:338: return video_frame; On 2015/05/14 at 00:58:57, DaleCurtis wrote: > I'd just do nothing here and change the return at the end of the function to always return video_frame. Done. https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:344: NOTREACHED(); On 2015/05/14 at 00:58:58, DaleCurtis wrote: > Again remove this, these falls under the unsupported cases. Done. https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool.h (right): https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.h:31: scoped_refptr<VideoFrame> MaybeCreateHardwareFrame( On 2015/05/14 at 00:58:58, DaleCurtis wrote: > Method needs comment. Done. Also rephrased the class comment.
lgtm on the media/ side of things. Please wait for reveman@ to approve. https://codereview.chromium.org/1133563010/diff/80001/media/renderers/video_r... File media/renderers/video_renderer_impl.cc (right): https://codereview.chromium.org/1133563010/diff/80001/media/renderers/video_r... media/renderers/video_renderer_impl.cc:57: new GpuMemoryBufferVideoFramePool(task_runner, gpu_factories)), On 2015/05/14 17:30:25, Daniele Castagna wrote: > On 2015/05/14 at 00:58:57, DaleCurtis wrote: > > Do you want this owned by each VideoRendererImpl (one per video tag) or would > it be better if the renderer owned a single pool that frames were served from? > > We were thinking about one per video tag right now. One per renderer would > probably be better to limit the total memory used though. > > Should we leave the one pool for renderer in another iteration? sgtm, but you guys are the experts :) https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:218: GpuMemoryBufferVideoFramePool::PoolImpl::FrameResources* On 2015/05/14 17:30:26, Daniele Castagna wrote: > On 2015/05/14 at 00:58:58, DaleCurtis wrote: > > Why not scoped_ptr? > > As per reveman's suggestion we decided to leave FrameResources ownership in the > pool and just mark the object as in_use. > Getting a FrameResouces gives you just a pointer to FrameResources and doesn't > transfer the ownership. > > This will make it easier to account memory usage of the pool and to deal with > context lost situations once we address that in media. I'm not sure about that logic, but up to you :) Someone calling this in the future may forget. https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:272: // TODO(dcastagna): As soon as the context lost is dealt with in media, On 2015/05/14 17:30:26, Daniele Castagna wrote: > On 2015/05/14 at 00:58:57, DaleCurtis wrote: > > Seems pretty important to fix sooner than later :) > > Absolutely. I talked with posciak@ about this and discovered that a gpu process > restart with hardware decoding would result in broken videos. :( > > I think we should deal with the context lost for media hardware decoding first, > and then this pool could follow. Depends on the failure mode, if it dangles use-after-frees then it has to be fixed before we can turn this on. https://codereview.chromium.org/1133563010/diff/100001/media/video/gpu_memory... File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1133563010/diff/100001/media/video/gpu_memory... media/video/gpu_memory_buffer_video_frame_pool.cc:69: static bool IsFrameResourcesCompatible(const FrameResources* resources, AreFrame...? https://codereview.chromium.org/1133563010/diff/100001/media/video/gpu_memory... File media/video/gpu_memory_buffer_video_frame_pool.h (right): https://codereview.chromium.org/1133563010/diff/100001/media/video/gpu_memory... media/video/gpu_memory_buffer_video_frame_pool.h:25: // in a rountrip to the browser/GPU process. round trip.
https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:218: GpuMemoryBufferVideoFramePool::PoolImpl::FrameResources* On 2015/05/14 at 18:52:13, DaleCurtis wrote: > On 2015/05/14 17:30:26, Daniele Castagna wrote: > > On 2015/05/14 at 00:58:58, DaleCurtis wrote: > > > Why not scoped_ptr? > > > > As per reveman's suggestion we decided to leave FrameResources ownership in the > > pool and just mark the object as in_use. > > Getting a FrameResouces gives you just a pointer to FrameResources and doesn't > > transfer the ownership. > > > > This will make it easier to account memory usage of the pool and to deal with > > context lost situations once we address that in media. > > I'm not sure about that logic, but up to you :) Someone calling this in the future may forget. I agree that it can be confusing, at least this method is private to PoolImpl... How would you change it? scoped_refptrs everywhere? https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:272: // TODO(dcastagna): As soon as the context lost is dealt with in media, On 2015/05/14 at 18:52:13, DaleCurtis wrote: > On 2015/05/14 17:30:26, Daniele Castagna wrote: > > On 2015/05/14 at 00:58:57, DaleCurtis wrote: > > > Seems pretty important to fix sooner than later :) > > > > Absolutely. I talked with posciak@ about this and discovered that a gpu process > > restart with hardware decoding would result in broken videos. :( > > > > I think we should deal with the context lost for media hardware decoding first, > > and then this pool could follow. > > Depends on the failure mode, if it dangles use-after-frees then it has to be fixed before we can turn this on. There is no use-after-frees and the resources are deleted when the context is lost. As the TODO says, we shouldn't even call this callback if the context is lost. https://codereview.chromium.org/1133563010/diff/100001/media/video/gpu_memory... File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1133563010/diff/100001/media/video/gpu_memory... media/video/gpu_memory_buffer_video_frame_pool.cc:69: static bool IsFrameResourcesCompatible(const FrameResources* resources, On 2015/05/14 at 18:52:13, DaleCurtis wrote: > AreFrame...? Done. https://codereview.chromium.org/1133563010/diff/100001/media/video/gpu_memory... File media/video/gpu_memory_buffer_video_frame_pool.h (right): https://codereview.chromium.org/1133563010/diff/100001/media/video/gpu_memory... media/video/gpu_memory_buffer_video_frame_pool.h:25: // in a rountrip to the browser/GPU process. On 2015/05/14 at 18:52:13, DaleCurtis wrote: > round trip. Done.
https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1133563010/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:218: GpuMemoryBufferVideoFramePool::PoolImpl::FrameResources* On 2015/05/14 19:24:16, Daniele Castagna wrote: > On 2015/05/14 at 18:52:13, DaleCurtis wrote: > > On 2015/05/14 17:30:26, Daniele Castagna wrote: > > > On 2015/05/14 at 00:58:58, DaleCurtis wrote: > > > > Why not scoped_ptr? > > > > > > As per reveman's suggestion we decided to leave FrameResources ownership in > the > > > pool and just mark the object as in_use. > > > Getting a FrameResouces gives you just a pointer to FrameResources and > doesn't > > > transfer the ownership. > > > > > > This will make it easier to account memory usage of the pool and to deal > with > > > context lost situations once we address that in media. > > > > I'm not sure about that logic, but up to you :) Someone calling this in the > future may forget. > > I agree that it can be confusing, at least this method is private to PoolImpl... > > How would you change it? scoped_refptrs everywhere? I was expected that resources_pool_ would be a ScopedVector, it's fine to return a raw pointer here though. Vector is fine if you use std::remove_if to swap elements to the back for removal to avoid erases at places other than the end.
lgtm https://codereview.chromium.org/1133563010/diff/160001/media/video/gpu_memory... File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1133563010/diff/160001/media/video/gpu_memory... media/video/gpu_memory_buffer_video_frame_pool.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. nit: no "(c)" in new files https://codereview.chromium.org/1133563010/diff/160001/media/video/gpu_memory... media/video/gpu_memory_buffer_video_frame_pool.cc:279: // make sure that we won't execture this callback (use a weak pointer to nit: s/execture/execute/ https://codereview.chromium.org/1133563010/diff/160001/media/video/gpu_memory... File media/video/gpu_memory_buffer_video_frame_pool.h (right): https://codereview.chromium.org/1133563010/diff/160001/media/video/gpu_memory... media/video/gpu_memory_buffer_video_frame_pool.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. nit: no "(c)" in new files
https://codereview.chromium.org/1133563010/diff/160001/media/video/gpu_memory... File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1133563010/diff/160001/media/video/gpu_memory... media/video/gpu_memory_buffer_video_frame_pool.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/05/15 at 05:44:03, reveman wrote: > nit: no "(c)" in new files Done. https://codereview.chromium.org/1133563010/diff/160001/media/video/gpu_memory... media/video/gpu_memory_buffer_video_frame_pool.cc:279: // make sure that we won't execture this callback (use a weak pointer to On 2015/05/15 at 05:44:03, reveman wrote: > nit: s/execture/execute/ Done. https://codereview.chromium.org/1133563010/diff/160001/media/video/gpu_memory... File media/video/gpu_memory_buffer_video_frame_pool.h (right): https://codereview.chromium.org/1133563010/diff/160001/media/video/gpu_memory... media/video/gpu_memory_buffer_video_frame_pool.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/05/15 at 05:44:03, reveman wrote: > nit: no "(c)" in new files Done.
The CQ bit was checked by dcastagna@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, reveman@chromium.org Link to the patchset: https://codereview.chromium.org/1133563010/#ps180001 (title: "Address reveman@'s nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1133563010/180001
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/612cc633d1e789d92208bdacdfa22eac5a4c800a Cr-Commit-Position: refs/heads/master@{#330053} |
