|
|
DescriptionSupport Memory-infra for MMAP memory allocated in V4L2 (S)VDA/VEA on CrOS
Track and dump driver-allocated MMAP memory in V4L2 (S)VDA/VEA.
Show that memory usage in chrome://tracing (memory-infra).
BUG=chromium:721674
TEST=seeing chrome://tracing (memory-infra)
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Patch Set 1 #
Total comments: 12
Patch Set 2 : Support Memory-infra for HW video codec allocations on CrOS #Patch Set 3 : Support Memory-infra for HW video codec allocations on CrOS #
Total comments: 26
Patch Set 4 : Support Memory-infra for HW video codec allocations on CrOS #Patch Set 5 : Support Memory-infra for HW video codec allocations on CrOS #
Total comments: 57
Patch Set 6 : Support Memory-infra for HW video codec allocations on CrOS #
Total comments: 87
Patch Set 7 : Support Memory-infra for HW video codec allocations on CrOS #
Total comments: 42
Patch Set 8 : Support Memory-infra for HW video codec allocations on CrOS #Patch Set 9 : Support Memory-infra for HW video codec allocations on CrOS #Patch Set 10 : Support Memory-infra for HW video codec allocations on CrOS #Patch Set 11 : Support Memory-infra for HW video codec allocations on CrOS #Patch Set 12 : Support Memory-infra for HW video codec allocations on CrOS #Patch Set 13 : Support Memory-infra for MMAP memory allocated in V4L2 (S)VDA/VEA on CrOS #Patch Set 14 : Support Memory-infra for MMAP memory allocated in V4L2 (S)VDA/VEA on CrOS #Patch Set 15 : Support Memory-infra for MMAP memory allocated in V4L2 (S)VDA/VEA on CrOS #
Total comments: 51
Patch Set 16 : Support Memory-infra for MMAP memory allocated in V4L2 (S)VDA/VEA on CrOS #
Total comments: 25
Patch Set 17 : Support Memory-infra for MMAP memory allocated in V4L2 (S)VDA/VEA on CrOS #Patch Set 18 : Support Memory-infra for MMAP memory allocated in V4L2 (S)VDA/VEA on CrOS #
Total comments: 12
Patch Set 19 : Support Memory-infra for MMAP memory allocated in V4L2 (S)VDA/VEA on CrOS #Patch Set 20 : Support Memory-infra for MMAP memory allocated in V4L2 (S)VDA/VEA on CrOS #Patch Set 21 : Support Memory-infra for MMAP memory allocated in V4L2 (S)VDA/VEA on CrOS #Patch Set 22 : Support Memory-infra for MMAP memory allocated in V4L2 (S)VDA/VEA on CrOS #Patch Set 23 : Support Memory-infra for MMAP memory allocated in V4L2 (S)VDA/VEA on CrOS #
Total comments: 10
Patch Set 24 : Support Memory-infra for MMAP memory allocated in V4L2 (S)VDA/VEA on CrOS #
Total comments: 4
Patch Set 25 : Support Memory-infra for MMAP memory allocated in V4L2 (S)VDA/VEA on CrOS #Patch Set 26 : Support Memory-infra for MMAP memory allocated in V4L2 (S)VDA/VEA on CrOS #
Total comments: 12
Patch Set 27 : Support Memory-infra for MMAP memory allocated in V4L2 (S)VDA/VEA on CrOS #
Total comments: 108
Patch Set 28 : Support Memory-infra for MMAP memory allocated in V4L2 (S)VDA/VEA on CrOS #Patch Set 29 : Support Memory-infra for MMAP memory allocated in V4L2 (S)VDA/VEA on CrOS #Patch Set 30 : Support Memory-infra for MMAP memory allocated in V4L2 (S)VDA/VEA on CrOS #Patch Set 31 : Support Memory-infra for MMAP memory allocated in V4L2 (S)VDA/VEA on CrOS #Patch Set 32 : Support Memory-infra for MMAP memory allocated in V4L2 (S)VDA/VEA on CrOS #Patch Set 33 : Support Memory-infra for MMAP memory allocated in V4L2 (S)VDA/VEA on CrOS #
Total comments: 48
Patch Set 34 : Support Memory-infra for MMAP memory allocated in V4L2 (S)VDA/VEA on CrOS #Messages
Total messages: 100 (45 generated)
Description was changed from ========== Support Memory-infra for HW video codec allocations on CrOS Show driver-allocated memory (e.g. MMAP memory and VASurface). BUG=chromium:721674 TEST=seeing memory-infra ========== to ========== Support Memory-infra for HW video codec allocations on CrOS Show driver-allocated memory (e.g. MMAP memory and VASurface). BUG=chromium:721674 TEST=seeing memory-infra CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== Support Memory-infra for HW video codec allocations on CrOS Show driver-allocated memory (e.g. MMAP memory and VASurface). BUG=chromium:721674 TEST=seeing memory-infra CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Support Memory-infra for HW video codec allocations on CrOS Show driver-allocated memory (e.g. MMAP memory and VASurface). BUG=chromium:721674 TEST=seeing memory-infra CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
hiroh@chromium.org changed reviewers: + posciak+watch@chromium.org, reveman+watch@chromium.org
Please see my document (https://docs.google.com/a/google.com/document/d/1XWN2camYBx7pbrXe-eZpvNQLS357...). Because it is WIP, there are probably some mistakes. I started driver-allocated memories first. This patch is the implementation for dumping MMAP memory. However, the current implementation does not work correctly. Memory-infra showed Video Codec wasted 0.0MiB, which actually 32MiB. The below is the log at V4L2MemoryManager::OnMemoryDump in V4L2VDA. I don't know why OnMemoryDump reports 0. Is there any bug in my implementation? [9635:9924:0526/152313.175424:ERROR:v4l2_video_decode_accelerator.cc(127)] Video Codec size 33554432 [9635:9849:0526/152355.600051:ERROR:v4l2_video_decode_accelerator.cc(99)] Video Codec size [OnMemoryDump] 0 [9635:9849:0526/152355.600179:ERROR:v4l2_video_decode_accelerator.cc(99)] Video Codec size [OnMemoryDump] 33554432 [9635:9849:0526/152355.831358:ERROR:v4l2_video_decode_accelerator.cc(99)] Video Codec size [OnMemoryDump] 0 [9635:9849:0526/152355.831483:ERROR:v4l2_video_decode_accelerator.cc(99)] Video Codec size [OnMemoryDump] 33554432 [9635:9849:0526/152356.086720:ERROR:v4l2_video_decode_accelerator.cc(99)] Video Codec size [OnMemoryDump] 0 [9635:9849:0526/152356.086918:ERROR:v4l2_video_decode_accelerator.cc(99)] Video Codec size [OnMemoryDump] 33554432 [9635:9849:0526/152356.353201:ERROR:v4l2_video_decode_accelerator.cc(99)] Video Codec size [OnMemoryDump] 0 [9635:9849:0526/152356.353325:ERROR:v4l2_video_decode_accelerator.cc(99)] Video Codec size [OnMemoryDump] 33554432 [9635:9849:0526/152356.596313:ERROR:v4l2_video_decode_accelerator.cc(99)] Video Codec size [OnMemoryDump] 0 [9635:9849:0526/152356.596433:ERROR:v4l2_video_decode_accelerator.cc(99)] Video Codec size [OnMemoryDump] 33554432.
reveman@chromium.org changed reviewers: + reveman@chromium.org
https://codereview.chromium.org/2909533003/diff/1/media/gpu/v4l2_video_decode... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2909533003/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:75: this, "V4L2MemoryDumpProvider", nullptr); "V4L2MemoryDumpProvider": The name should be unique and describe the memory. you're creating multiple instances of this class so it's not and "DumpProvider" is not useful description to people debugging memory usage. nullptr: null as task runner means that this is thread safe and OnMemoryDump can be called on any thread. That's not the case with your current implementation and I would avoid making it thread safe for performance reasons. https://codereview.chromium.org/2909533003/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:91: bool V4L2VideoDecodeAccelerator::V4L2MemoryManager::OnMemoryDump( Can you provide more details in the dump? Number of objects, wasted space from padding, etc. https://codereview.chromium.org/2909533003/diff/1/media/gpu/v4l2_video_decode... File media/gpu/v4l2_video_decode_accelerator.h (right): https://codereview.chromium.org/2909533003/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.h:585: class V4L2MemoryManager why have a separate class instead of having V4L2VideoDecodeAccelerator implement the MemoryDumpProvider interface? https://codereview.chromium.org/2909533003/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.h:590: void IncrementMemoryUsage(size_t size); why is this public function needed that only does "memory_usage_ += size;"? https://codereview.chromium.org/2909533003/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.h:591: void ResetMemoryUsage(); and same here. I'd expect a memory manager to take care of this when Allocate and Deallocate is called. https://codereview.chromium.org/2909533003/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.h:592: bool Allocate(struct v4l2_requestbuffers *reqbufs, Can this return a std::unique_ptr<> of some kind that represent the lifetime of the buffer? Makes it so much easier to be confident that we're not leaking memory. What happens if Allocate is called twice and then Deallocate once? https://codereview.chromium.org/2909533003/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.h:593: scoped_refptr<V4L2Device> device_); nit: s/device_/device/ Why pass ownership of device to this function. That results in ref count churn. Use "V4L2Device* device" or "const scoped_refptr<V4L2Device>& device" to avoid it but I wonder if it's not better to pass a V4L2Device* to the ctor and store a reference in a scoped_refptr<V4L2Device> member? https://codereview.chromium.org/2909533003/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.h:594: bool Deallocate(struct v4l2_requestbuffers *reqbufs, how can deallocate fail and what is the caller supposed to do in this case? https://codereview.chromium.org/2909533003/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.h:597: ~V4L2MemoryManager() override; nit: move ctor/dtor to top https://codereview.chromium.org/2909533003/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.h:599: size_t memory_usage_; memory_usage_ = 0; here instead of memory_usage_(0) in ctor? https://codereview.chromium.org/2909533003/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.h:600: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2909533003/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.h:603: V4L2MemoryManager output_memory_manager_; Why do you have two of these if they allocate the same type of memory? If you want to describe if the memory is used for input or output buffers then VideoDecodeAccelerator that is aware of this can provides dumps with this info.
I change the design as V4L2Device has a responsibility of V4L2VDA Input/Output Buffers. There is still a bug. The amount of memory cannot be computed precisely due to the failure of QUERY_BUF.
I'm really confused by this design. Looks like you're creating a memory dump provider per allocation. Does that mean we'll have multiple of these when we have multiple videos? The dump provider instance is supposed to represent one allocator or manager of memory for a specific type of memory. E.g. discardable-shared-memory-manager, browser-gpu-memory-buffer-manager, etc. Why are we not creating one manager instance for all this memory that can provide memory dumps?
hiroh@chromium.org changed reviewers: + posciak@chromium.org - posciak+watch@chromium.org, reveman+watch@chromium.org
New design proposal is uploaded. Because this is design proposal, I didn't compile yet. I'm sorry if there is a typo.
https://codereview.chromium.org/2909533003/diff/40001/media/gpu/v4l2_mmap_man... File media/gpu/v4l2_mmap_manager.h (right): https://codereview.chromium.org/2909533003/diff/40001/media/gpu/v4l2_mmap_man... media/gpu/v4l2_mmap_manager.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2909533003/diff/40001/media/gpu/v4l2_mmap_man... media/gpu/v4l2_mmap_manager.h:5: #ifndef V4L2_MMAP_MANAGER_H Please use guards according to coding style. https://codereview.chromium.org/2909533003/diff/40001/media/gpu/v4l2_mmap_man... media/gpu/v4l2_mmap_manager.h:8: class MEDIA_GPU_EXPORT V4L2MmapMemoryManager { This must derive from RefCounted (please see https://cs.chromium.org/chromium/src/base/memory/ref_counted.h?type=cs&l=381 on how to use scoped_refptrs). https://codereview.chromium.org/2909533003/diff/40001/media/gpu/v4l2_mmap_man... media/gpu/v4l2_mmap_manager.h:8: class MEDIA_GPU_EXPORT V4L2MmapMemoryManager { Perhaps V4L2MmapMemory, or V4L2MmapAllocation instead of V4L2MmapMemoryManager? Please add a comment what this class represents and what its role is. https://codereview.chromium.org/2909533003/diff/40001/media/gpu/v4l2_mmap_man... media/gpu/v4l2_mmap_manager.h:19: static V4L2MMapMemoryManager* Create(const scoped_refptr<V4L2Device>& device, This should return a scoped_refptr. https://codereview.chromium.org/2909533003/diff/40001/media/gpu/v4l2_mmap_man... media/gpu/v4l2_mmap_manager.h:19: static V4L2MMapMemoryManager* Create(const scoped_refptr<V4L2Device>& device, The preferred way of passing scoped_refptr<> is by value (please see https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md). https://codereview.chromium.org/2909533003/diff/40001/media/gpu/v4l2_mmap_man... media/gpu/v4l2_mmap_manager.h:20: v4l2_requestbuffers* reqbufs, Const reference instead of pointer please. https://codereview.chromium.org/2909533003/diff/40001/media/gpu/v4l2_mmap_man... media/gpu/v4l2_mmap_manager.h:22: Callback<void(size_t)> UnRegisterMemory); Since you plan to have a common VideoMemoryManager used not only for V4L2 allocations, VideoMemoryManager should preferably not know about any V4L2 specifics, like it has to right now. I would suggest removing CreateV4L2MmapMemoryManager() from VideoMemoryManager completely and perhaps doing something like this: scoped_refptr<V4L2MMapMemoryManager> V4L2MMapMemoryManager::Create(scoped_refptr<V4L2Device> device, const struct v4l2_requestbuffers& reqbufs) { scoped_refptr<V4L2MMapMemoryManager> mem(new V4L2MMapMemoryManager(device)); // Allocate() should return 0 if allocation failed. size_t allocated_size = mem->Allocate(reqbufs); if (!allocated_size) return nullptr; VideoMemoryManager* mem_manager = VideoMemoryManager::GetInstance(); auto unregister_cb = mem_manager->RegisterAllocation(allocated_size); mem->AddDestructionCallback(unregister_cb); return mem; } V4L2MMapMemoryManager::AddDestructionCallback(const base::Closure& cb) { destruction_cb_ = cb; } V4L2MMapMemoryManager::~V4L2MMapMemoryManager() { Deallocate(); if (!destruction_cb_.is_null()) destruction_cb_.Run(); } base::Closure VideoMemoryManager::RegisterAllocation(size_t size) { IncreaseMemoryUsage(size); return base::BindOnce(&VideoMemoryManager::DecreaseMemoryUsage, base::Unretained(this), size)); } https://codereview.chromium.org/2909533003/diff/40001/media/gpu/v4l2_mmap_man... media/gpu/v4l2_mmap_manager.h:37: // This class must be an owner of device so that be an owner -> take a reference to https://codereview.chromium.org/2909533003/diff/40001/media/gpu/v4l2_mmap_man... media/gpu/v4l2_mmap_manager.h:43: v4l2_requestbuffers reqbufs_; const
I like the idea of having some form of VideoMemoryManager but do we need multiple managers? Is it not enough to have one manager that tracks all memory and provides appropriate memory infra dumps? https://codereview.chromium.org/2909533003/diff/40001/media/gpu/video_memory_... File media/gpu/video_memory_manager.h (right): https://codereview.chromium.org/2909533003/diff/40001/media/gpu/video_memory_... media/gpu/video_memory_manager.h:18: static MemoryDumpManager* GetInstance(); VideoMemoryManager* https://codereview.chromium.org/2909533003/diff/40001/media/gpu/video_memory_... media/gpu/video_memory_manager.h:26: void IncreaseMemoryUsage(size_t sz); why is this and below function not internal to the implementation? https://codereview.chromium.org/2909533003/diff/40001/media/gpu/video_memory_... media/gpu/video_memory_manager.h:34: static V4L2MmapMemoryManager* CreateV4L2MmapMemoryManager( why is this static and why does it allocate a memory manager and not memory? also please return a std::unique_ptr
I improved the design referring the comments and also implemented the functions. No test is done yet, though the compilation is success. https://codereview.chromium.org/2909533003/diff/40001/media/gpu/v4l2_mmap_man... File media/gpu/v4l2_mmap_manager.h (right): https://codereview.chromium.org/2909533003/diff/40001/media/gpu/v4l2_mmap_man... media/gpu/v4l2_mmap_manager.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2017/06/02 08:05:11, Pawel Osciak wrote: > 2017 Done. https://codereview.chromium.org/2909533003/diff/40001/media/gpu/v4l2_mmap_man... media/gpu/v4l2_mmap_manager.h:5: #ifndef V4L2_MMAP_MANAGER_H On 2017/06/02 08:05:11, Pawel Osciak wrote: > Please use guards according to coding style. Done. https://codereview.chromium.org/2909533003/diff/40001/media/gpu/v4l2_mmap_man... media/gpu/v4l2_mmap_manager.h:8: class MEDIA_GPU_EXPORT V4L2MmapMemoryManager { On 2017/06/02 08:05:10, Pawel Osciak wrote: > This must derive from RefCounted (please see > https://cs.chromium.org/chromium/src/base/memory/ref_counted.h?type=cs&l=381 on > how to use scoped_refptrs). Instead of unique_ptr, this instance is used through unique_ptr. Therefore, RefCounted is unnecessary. https://codereview.chromium.org/2909533003/diff/40001/media/gpu/v4l2_mmap_man... media/gpu/v4l2_mmap_manager.h:8: class MEDIA_GPU_EXPORT V4L2MmapMemoryManager { On 2017/06/02 08:05:11, Pawel Osciak wrote: > Perhaps V4L2MmapMemory, or V4L2MmapAllocation instead of V4L2MmapMemoryManager? > > Please add a comment what this class represents and what its role is. Done. https://codereview.chromium.org/2909533003/diff/40001/media/gpu/v4l2_mmap_man... media/gpu/v4l2_mmap_manager.h:19: static V4L2MMapMemoryManager* Create(const scoped_refptr<V4L2Device>& device, On 2017/06/02 08:05:11, Pawel Osciak wrote: > The preferred way of passing scoped_refptr<> is by value (please see > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md). Done. https://codereview.chromium.org/2909533003/diff/40001/media/gpu/v4l2_mmap_man... media/gpu/v4l2_mmap_manager.h:19: static V4L2MMapMemoryManager* Create(const scoped_refptr<V4L2Device>& device, On 2017/06/02 08:05:11, Pawel Osciak wrote: > This should return a scoped_refptr. I change let this return unique_ptr instead of a scoped_refptr. https://codereview.chromium.org/2909533003/diff/40001/media/gpu/v4l2_mmap_man... media/gpu/v4l2_mmap_manager.h:20: v4l2_requestbuffers* reqbufs, On 2017/06/02 08:05:11, Pawel Osciak wrote: > Const reference instead of pointer please. Done. https://codereview.chromium.org/2909533003/diff/40001/media/gpu/v4l2_mmap_man... media/gpu/v4l2_mmap_manager.h:22: Callback<void(size_t)> UnRegisterMemory); On 2017/06/02 08:05:11, Pawel Osciak wrote: > Since you plan to have a common VideoMemoryManager used not only for V4L2 > allocations, VideoMemoryManager should preferably not know about any V4L2 > specifics, like it has to right now. > > I would suggest removing CreateV4L2MmapMemoryManager() from VideoMemoryManager > completely and perhaps doing something like this: > > > scoped_refptr<V4L2MMapMemoryManager> > V4L2MMapMemoryManager::Create(scoped_refptr<V4L2Device> device, > const struct v4l2_requestbuffers& reqbufs) { > scoped_refptr<V4L2MMapMemoryManager> mem(new V4L2MMapMemoryManager(device)); > // Allocate() should return 0 if allocation failed. > size_t allocated_size = mem->Allocate(reqbufs); > if (!allocated_size) > return nullptr; > > VideoMemoryManager* mem_manager = VideoMemoryManager::GetInstance(); > auto unregister_cb = > mem_manager->RegisterAllocation(allocated_size); > mem->AddDestructionCallback(unregister_cb); > return mem; > } > > V4L2MMapMemoryManager::AddDestructionCallback(const base::Closure& cb) { > destruction_cb_ = cb; > } > > V4L2MMapMemoryManager::~V4L2MMapMemoryManager() { > Deallocate(); > if (!destruction_cb_.is_null()) > destruction_cb_.Run(); > } > > base::Closure VideoMemoryManager::RegisterAllocation(size_t size) { > IncreaseMemoryUsage(size); > return base::BindOnce(&VideoMemoryManager::DecreaseMemoryUsage, > base::Unretained(this), size)); > } Done. https://codereview.chromium.org/2909533003/diff/40001/media/gpu/v4l2_mmap_man... media/gpu/v4l2_mmap_manager.h:37: // This class must be an owner of device so that On 2017/06/02 08:05:11, Pawel Osciak wrote: > be an owner -> take a reference to Done. https://codereview.chromium.org/2909533003/diff/40001/media/gpu/v4l2_mmap_man... media/gpu/v4l2_mmap_manager.h:43: v4l2_requestbuffers reqbufs_; On 2017/06/02 08:05:11, Pawel Osciak wrote: > const Done. https://codereview.chromium.org/2909533003/diff/40001/media/gpu/video_memory_... File media/gpu/video_memory_manager.h (right): https://codereview.chromium.org/2909533003/diff/40001/media/gpu/video_memory_... media/gpu/video_memory_manager.h:18: static MemoryDumpManager* GetInstance(); On 2017/06/02 14:40:07, reveman wrote: > VideoMemoryManager* Done. https://codereview.chromium.org/2909533003/diff/40001/media/gpu/video_memory_... media/gpu/video_memory_manager.h:26: void IncreaseMemoryUsage(size_t sz); On 2017/06/02 14:40:07, reveman wrote: > why is this and below function not internal to the implementation? Done. https://codereview.chromium.org/2909533003/diff/40001/media/gpu/video_memory_... media/gpu/video_memory_manager.h:34: static V4L2MmapMemoryManager* CreateV4L2MmapMemoryManager( On 2017/06/02 14:40:06, reveman wrote: > why is this static and why does it allocate a memory manager and not memory? > also please return a std::unique_ptr Done.
I add locks in several functions in VideoMemoryManager.
First pass. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/BUILD.gn File media/gpu/BUILD.gn (right): https://codereview.chromium.org/2909533003/diff/80001/media/gpu/BUILD.gn#newc... media/gpu/BUILD.gn:144: "v4l2_mmap_memory.cc", The v4l2_* files should only be included if we are using V4L2, i.e. use_v4l2_codec is defined. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... File media/gpu/v4l2_mmap_memory.cc (right): https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.cc:5: #include <cstring> Is this needed? https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.cc:49: struct v4l2_plane planes[1]; Please use struct v4l2_plane planes[VIDEO_MAX_PLANES] and iterate over the returned buffer.length (e.g. https://cs.chromium.org/chromium/src/media/gpu/v4l2_jpeg_decode_accelerator.c...). https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.cc:59: buffer.length = 1; arraysize(planes) https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.cc:72: std::memcpy(&reqbufs, &reqbufs_, sizeof(v4l2_requestbuffers)); v4l2_requestbuffers reqbufs = reqbufs_; https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.cc:74: device_->Ioctl(VIDIOC_REQBUFS, &reqbufs); I'd add a log for failures here. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... File media/gpu/v4l2_mmap_memory.h (right): https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.h:8: #include "base/callback.h" Would just "base/callback_forward.h" work here as well? https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.h:14: class MEDIA_GPU_EXPORT V4L2MmapMemory { Please document the class. Is MEDIA_GPU_EXPORT needed? https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.h:19: // During this function, the memory usage in VideoMemoryManager is increased I'd suggest that clients of this class should not have to be interested in/concerned about this class registering/tracking the allocations. The role of this class for them is to be a source of memory. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.h:23: v4l2_requestbuffers* const reqbufs, const reference? https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.h:24: VideoMemoryManager::MemoryType type); What type of memory for VMM this class (or any other video memory type class) produces should probably not have to be a property set by the client, but by this class itself, so I'd remove this argument and instead have a constant in this class. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.h:26: V4L2MmapMemory() = default; As Create() should be the only way to instantiate this class, the default constructor should be protected/private ( DISALLOW_IMPLICIT_CONSTRUCTORS() could be useful here). https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.h:31: v4l2_requestbuffers* const reqbufs); const ref https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.h:37: // This function must be called only withn Create() s/withn/within/ Do we need to say that this can only be called within Create? The ioctl should fail otherwise, and it's also a private method. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.h:38: size_t Allocate(v4l2_requestbuffers* const reqbufs); const ref https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.h:41: // This function must be called only within destructor Ditto. If we really care about saying this, we could also just move the body of the above methods into Create() and the destructor. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.h:45: void InitDestructionCallBack(base::OnceClosure&& cb); I think we should be receiving OnceClosure by value if taking ownership? https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.h:47: // This class must take a refernece to device so that s/refernece/reference/ https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_video_de... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.cc:188: input_buffer_memory(nullptr), unique_ptr is initialized to nullptr in default constructor. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.cc:376: // IOCTL_OR_ERROR_RETURN(VIDIOC_REQBUFS, &reqbufs); Please remove. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.cc:378: VideoMemoryManager::GetInstance()->CreateV4L2MmapMemory( V4L2MmapMemory::Create() https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.cc:382: if (reqbufs.count != buffers.size()) { Should we check here if CreateV4L2MmapMemory() returned non-nullptr and instead have this check in CreateV4L2MmapMemory()? https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.cc:2164: // IOCTL_OR_ERROR_RETURN_FALSE(VIDIOC_REQBUFS, &reqbufs); Please remove. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_video_de... File media/gpu/v4l2_video_decode_accelerator.h (right): https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.h:32: #include "media/gpu/v4l2_mmap_memory.h" Just forward declaration should work here, with include in .cc. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.h:581: // V4L2MMapMemoryManager for input buffer Please update documentation. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/video_memory_... File media/gpu/video_memory_manager.cc (right): https://codereview.chromium.org/2909533003/diff/80001/media/gpu/video_memory_... media/gpu/video_memory_manager.cc:41: reportMemoryUsage("V4L2 VDA Input Buffer", size); Could these strings come from the memory type instead? https://codereview.chromium.org/2909533003/diff/80001/media/gpu/video_memory_... media/gpu/video_memory_manager.cc:60: std::unique_ptr<V4L2MmapMemory> VideoMemoryManager::CreateV4L2MmapMemory( Ideally I think VMM shouldn't have to know details of different memory types and not have a Create method for each. Each memory type class could get an instance to VMM directly and register itself from its own Create. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/video_memory_... File media/gpu/video_memory_manager.h (right): https://codereview.chromium.org/2909533003/diff/80001/media/gpu/video_memory_... media/gpu/video_memory_manager.h:8: #include <linux/videodev2.h> This could be in .cc? Please in general try to include as little as possible from header files.
VideoMemoryManager manages the size and name of the memory. Each memory is represented by a unique index. SequenceChecker in VideoMemoryManager guarantees that index and the data structure are accessed by the same thread. As a result, lock is unnecessary. Although Compile is success, I confirmed VIDIOC_QUERYBUF was failed in the case of "V4L2 Output Buffer." https://codereview.chromium.org/2909533003/diff/80001/media/gpu/BUILD.gn File media/gpu/BUILD.gn (right): https://codereview.chromium.org/2909533003/diff/80001/media/gpu/BUILD.gn#newc... media/gpu/BUILD.gn:144: "v4l2_mmap_memory.cc", On 2017/06/05 07:37:00, Pawel Osciak wrote: > The v4l2_* files should only be included if we are using V4L2, i.e. > use_v4l2_codec is defined. Done. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... File media/gpu/v4l2_mmap_memory.cc (right): https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.cc:5: #include <cstring> On 2017/06/05 07:37:00, Pawel Osciak wrote: > Is this needed? Done. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.cc:49: struct v4l2_plane planes[1]; On 2017/06/05 07:37:00, Pawel Osciak wrote: > Please use struct v4l2_plane planes[VIDEO_MAX_PLANES] and iterate over the > returned buffer.length (e.g. > https://cs.chromium.org/chromium/src/media/gpu/v4l2_jpeg_decode_accelerator.c...). Done. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.cc:59: buffer.length = 1; On 2017/06/05 07:37:00, Pawel Osciak wrote: > arraysize(planes) Can VIDOC_MAX_PLANES be simply? https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.cc:72: std::memcpy(&reqbufs, &reqbufs_, sizeof(v4l2_requestbuffers)); On 2017/06/05 07:37:00, Pawel Osciak wrote: > v4l2_requestbuffers reqbufs = reqbufs_; Done. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.cc:74: device_->Ioctl(VIDIOC_REQBUFS, &reqbufs); On 2017/06/05 07:37:00, Pawel Osciak wrote: > I'd add a log for failures here. Done. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... File media/gpu/v4l2_mmap_memory.h (right): https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.h:8: #include "base/callback.h" On 2017/06/05 07:37:00, Pawel Osciak wrote: > Would just "base/callback_forward.h" work here as well? Done. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.h:14: class MEDIA_GPU_EXPORT V4L2MmapMemory { On 2017/06/05 07:37:01, Pawel Osciak wrote: > Please document the class. > > Is MEDIA_GPU_EXPORT needed? Done. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.h:19: // During this function, the memory usage in VideoMemoryManager is increased On 2017/06/05 07:37:01, Pawel Osciak wrote: > I'd suggest that clients of this class should not have to be interested > in/concerned about this class registering/tracking the allocations. The role of > this class for them is to be a source of memory. Done. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.h:23: v4l2_requestbuffers* const reqbufs, On 2017/06/05 07:37:00, Pawel Osciak wrote: > const reference? |reqbufs| is written by Ioctl(VIDIOC_REQBUFS, reqbufs) const reference is impossible. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.h:24: VideoMemoryManager::MemoryType type); On 2017/06/05 07:37:01, Pawel Osciak wrote: > What type of memory for VMM this class (or any other video memory type class) > produces should probably not have to be a property set by the client, but by > this class itself, so I'd remove this argument and instead have a constant in > this class. Done. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.h:26: V4L2MmapMemory() = default; On 2017/06/05 07:37:00, Pawel Osciak wrote: > As Create() should be the only way to instantiate this class, the default > constructor should be protected/private ( DISALLOW_IMPLICIT_CONSTRUCTORS() could > be useful here). Done. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.h:31: v4l2_requestbuffers* const reqbufs); On 2017/06/05 07:37:01, Pawel Osciak wrote: > const ref Done. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.h:37: // This function must be called only withn Create() On 2017/06/05 07:37:00, Pawel Osciak wrote: > s/withn/within/ > > Do we need to say that this can only be called within Create? The ioctl should > fail otherwise, and it's also a private method. Done. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.h:38: size_t Allocate(v4l2_requestbuffers* const reqbufs); On 2017/06/05 07:37:00, Pawel Osciak wrote: > const ref ditto. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.h:41: // This function must be called only within destructor On 2017/06/05 07:37:01, Pawel Osciak wrote: > Ditto. If we really care about saying this, we could also just move the body of > the above methods into Create() and the destructor. Done. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.h:45: void InitDestructionCallBack(base::OnceClosure&& cb); On 2017/06/05 07:37:00, Pawel Osciak wrote: > I think we should be receiving OnceClosure by value if taking ownership? Done. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.h:47: // This class must take a refernece to device so that On 2017/06/05 07:37:00, Pawel Osciak wrote: > s/refernece/reference/ Done. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_video_de... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.cc:188: input_buffer_memory(nullptr), On 2017/06/05 07:37:01, Pawel Osciak wrote: > unique_ptr is initialized to nullptr in default constructor. Done. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.cc:376: // IOCTL_OR_ERROR_RETURN(VIDIOC_REQBUFS, &reqbufs); On 2017/06/05 07:37:01, Pawel Osciak wrote: > Please remove. Done. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.cc:378: VideoMemoryManager::GetInstance()->CreateV4L2MmapMemory( On 2017/06/05 07:37:01, Pawel Osciak wrote: > V4L2MmapMemory::Create() Done. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.cc:382: if (reqbufs.count != buffers.size()) { On 2017/06/05 07:37:01, Pawel Osciak wrote: > Should we check here if CreateV4L2MmapMemory() returned non-nullptr and instead > have this check in CreateV4L2MmapMemory()? Done. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.cc:2164: // IOCTL_OR_ERROR_RETURN_FALSE(VIDIOC_REQBUFS, &reqbufs); On 2017/06/05 07:37:01, Pawel Osciak wrote: > Please remove. Done. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_video_de... File media/gpu/v4l2_video_decode_accelerator.h (right): https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.h:32: #include "media/gpu/v4l2_mmap_memory.h" On 2017/06/05 07:37:01, Pawel Osciak wrote: > Just forward declaration should work here, with include in .cc. Done. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.h:581: // V4L2MMapMemoryManager for input buffer On 2017/06/05 07:37:01, Pawel Osciak wrote: > Please update documentation. Done. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/video_memory_... File media/gpu/video_memory_manager.cc (right): https://codereview.chromium.org/2909533003/diff/80001/media/gpu/video_memory_... media/gpu/video_memory_manager.cc:41: reportMemoryUsage("V4L2 VDA Input Buffer", size); On 2017/06/05 07:37:01, Pawel Osciak wrote: > Could these strings come from the memory type instead? Done. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/video_memory_... media/gpu/video_memory_manager.cc:60: std::unique_ptr<V4L2MmapMemory> VideoMemoryManager::CreateV4L2MmapMemory( On 2017/06/05 07:37:01, Pawel Osciak wrote: > Ideally I think VMM shouldn't have to know details of different memory types and > not have a Create method for each. Each memory type class could get an instance > to VMM directly and register itself from its own Create. Done. https://codereview.chromium.org/2909533003/diff/80001/media/gpu/video_memory_... File media/gpu/video_memory_manager.h (right): https://codereview.chromium.org/2909533003/diff/80001/media/gpu/video_memory_... media/gpu/video_memory_manager.h:8: #include <linux/videodev2.h> On 2017/06/05 07:37:01, Pawel Osciak wrote: > This could be in .cc? Please in general try to include as little as possible > from header files. Done.
Add some comments. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... File media/gpu/video_memory_manager.cc (right): https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.cc:67: memory_name_[index] = name; The present implementation manages the information by a unique index instead of the type of memory. Both of data structures cannot have the data whose key |index|. So, size and name are just assigned to each data structure. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.cc:73: memory_name_.erase(index); Same as above, index is unique. memory_usage_[index] is unchanged after it is created. The data specified by index in both data structures are just erased.
add comments https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_video_d... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:2507: */ Should deallocation be executed clearly here by doing output_buffer_memory.reset(nullptr)? https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:2555: */ ditto.
https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... File media/gpu/v4l2_mmap_memory.h (right): https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_mem... media/gpu/v4l2_mmap_memory.h:23: v4l2_requestbuffers* const reqbufs, On 2017/06/06 05:55:09, hiroh wrote: > On 2017/06/05 07:37:00, Pawel Osciak wrote: > > const reference? > > |reqbufs| is written by Ioctl(VIDIOC_REQBUFS, reqbufs) > const reference is impossible. Yes, but you could make a copy of it when passing to kernel. On the other hand, the caller is not using the data returned in it (since we are checking count in Create(). Is the intended API usage to be able to get this as a return value? If so, please document that. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... File media/gpu/v4l2_mmap_memory.cc (right): https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:26: std::unique_ptr<V4L2MmapMemory> v4l2mm(new V4L2MmapMemory(device, reqbufs)); Perhaps pass reqbufs only to Allocate, and store it from there if we succeed the allocation? https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:68: buffer.length = VIDEO_MAX_PLANES; Using arraysize(planes) make this less error-prone if in the future the declaration of planes is updated, but this line is not. Both are compile-time. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:90: } // namepace media s/namepace/namespace/ https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... File media/gpu/v4l2_mmap_memory.h (right): https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:12: #include "base/memory/ref_counted.h" Is this used? https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:13: #include "media/gpu/v4l2_device.h" Could V4L2Device be a forward-declaration instead? https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:17: // This class manages the MMAP memory allocated in V4L2(S) VDA/VEA This class exposes V4L2 memory allocation capabilities, but it shouldn't specify what its clients are. This could suggest there was a reason classes other than mentioned above could not use this class. We can have other clients of it, in the future, or even now, like V4L2JDA. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:22: // in this destructor. s/this/the/ https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:28: // A new V4L2MmapMemory is constructed with |reqbufs| and |device|. Please specify why reqbufs is a pointer if you choose to use a pointer instead of const ref. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:29: // If the allocation (e.g. ioctl) is failed, this returns nullptr and We should mention that we only succeed if the exact reqbufs.count is allocated. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:34: const std::string& name); Please document the name argument. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:39: V4L2MmapMemory() = delete; Could we use DISALLOW_IMPLICIT_CONSTRUCTORS()? https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:47: // Before calling Allocate, device must be initilalized. s/initilalized/initialized/ https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:65: // a responsibility to it. This is called within destructor s/to it/for/ https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_video_d... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:378: LOG(ERROR) << "Output Buffer Create Error !"; NOTIFY_ERROR(PLATFORM_FAILURE); https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:2162: LOG(ERROR) << "Input Buffer Create Error"; NOTIFY_ERROR() https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:2165: input_buffer_map_.resize(reqbufs.count); We could also use kInputBufferCount since we check that driver allocated the exact number of buffers in Create(). https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:2507: */ On 2017/06/06 06:09:29, hiroh wrote: > Should deallocation be executed clearly here by doing > output_buffer_memory.reset(nullptr)? I would say so, yes. output_buffer_memory = nullptr; may be shorter. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_video_d... File media/gpu/v4l2_video_decode_accelerator.h (right): https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.h:583: std::unique_ptr<V4L2MmapMemory> input_buffer_memory; s/input_buffer_memory/input_buffer_memory_/ https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.h:586: std::unique_ptr<V4L2MmapMemory> output_buffer_memory; s/output_buffer_memory/output_buffer_memory_/ https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... File media/gpu/video_memory_manager.cc (right): https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.cc:43: int32_t index = mem.first; index is uint64_t? Perhaps use auto? https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.cc:56: uint64_t index = index_++; Could https://cs.chromium.org/chromium/src/gpu/command_buffer/common/id_type.h?l=49 be useful here? https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.cc:59: base::Unretained(this), index, size); Please add a comment explaining why Unretained is safe. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.cc:62: void VideoMemoryManager::RegisterMemoryData(uint64_t index, Perhaps we could inline this into RegisterAllocation? https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.cc:70: void VideoMemoryManager::UnregisterMemoryData(uint64_t index, size_t size) { size is unused? https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... File media/gpu/video_memory_manager.h (right): https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.h:14: #include "base/synchronization/lock.h" Not needed? https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.h:19: // This class trucks all the memory used in Video Codec. s/trucks/tracks/ https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.h:20: // The information of the memories dumped by this class can be shown s/memories/memory/ https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.h:24: // MMAP memory, VASurface Similarly, this could suggest no other clients could use this class. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.h:25: // TODO(hiroh): enumerates all the managed memory Please be more specific, so others could understand what is required to do here. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.h:27: // Classes in Video Codec do not call this class directlly. s/directlly/directly/ https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.h:30: class VideoMemoryManager : public base::trace_event::MemoryDumpProvider { Please document that all methods of this class must be called on the same thread. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.h:40: // Return Callback function to decrease the memory usage by |size| Please update this comment. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.h:58: std::unordered_map<uint64_t, size_t> memory_usage_; Could we have one, common map storing a struct of size and string? That would save us a lookup and the need to maintain consistency between two maps. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.h:62: // index_ represensts each memory type and is uqniue to each memory s/represensts/represents/ s/uqniue/unique/ https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.h:64: // index increases monotonically Nit: please format all comment sentences in this CL to start with a capital letter and end with a dot. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.h:67: // By using SequenceChecker, it is guranteed that memory_usage_, s/guranteed/guaranteed/ https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.h:69: SEQUENCE_CHECKER(sequence_chcker_); s/sequence_chcker_/sequence_checker_/
https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... File media/gpu/v4l2_mmap_memory.cc (right): https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:26: std::unique_ptr<V4L2MmapMemory> v4l2mm(new V4L2MmapMemory(device, reqbufs)); On 2017/06/07 10:08:11, Pawel Osciak wrote: > Perhaps pass reqbufs only to Allocate, and store it from there if we succeed the > allocation? Yes. Therefore, a pointer should be used to pass an argument to this function. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:68: buffer.length = VIDEO_MAX_PLANES; On 2017/06/07 10:08:11, Pawel Osciak wrote: > Using arraysize(planes) make this less error-prone if in the future the > declaration of planes is updated, but this line is not. Both are compile-time. Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:90: } // namepace media On 2017/06/07 10:08:11, Pawel Osciak wrote: > s/namepace/namespace/ Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... File media/gpu/v4l2_mmap_memory.h (right): https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:12: #include "base/memory/ref_counted.h" On 2017/06/07 10:08:12, Pawel Osciak wrote: > Is this used? scoped_refptr is defined in this header file. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:13: #include "media/gpu/v4l2_device.h" On 2017/06/07 10:08:11, Pawel Osciak wrote: > Could V4L2Device be a forward-declaration instead? Done. After removing v4l2_device.h, the compilation is failed due to OnceClosure. Because of this, I change to include callback.h instead of callback.h https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:17: // This class manages the MMAP memory allocated in V4L2(S) VDA/VEA On 2017/06/07 10:08:11, Pawel Osciak wrote: > This class exposes V4L2 memory allocation capabilities, but it shouldn't specify > what its clients are. This could suggest there was a reason classes other than > mentioned above could not use this class. > We can have other clients of it, in the future, or even now, like V4L2JDA. Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:22: // in this destructor. On 2017/06/07 10:08:11, Pawel Osciak wrote: > s/this/the/ Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:28: // A new V4L2MmapMemory is constructed with |reqbufs| and |device|. On 2017/06/07 10:08:11, Pawel Osciak wrote: > Please specify why reqbufs is a pointer if you choose to use a pointer instead > of const ref. According to chromium style guide, a pointer should be used if it is written within the function. I write a comment that reqbufs are written in this function. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:29: // If the allocation (e.g. ioctl) is failed, this returns nullptr and On 2017/06/07 10:08:11, Pawel Osciak wrote: > We should mention that we only succeed if the exact reqbufs.count is allocated. Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:34: const std::string& name); On 2017/06/07 10:08:11, Pawel Osciak wrote: > Please document the name argument. Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:39: V4L2MmapMemory() = delete; On 2017/06/07 10:08:11, Pawel Osciak wrote: > Could we use DISALLOW_IMPLICIT_CONSTRUCTORS()? Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:47: // Before calling Allocate, device must be initilalized. On 2017/06/07 10:08:11, Pawel Osciak wrote: > s/initilalized/initialized/ Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:65: // a responsibility to it. This is called within destructor On 2017/06/07 10:08:11, Pawel Osciak wrote: > s/to it/for/ Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_video_d... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:378: LOG(ERROR) << "Output Buffer Create Error !"; On 2017/06/07 10:08:12, Pawel Osciak wrote: > NOTIFY_ERROR(PLATFORM_FAILURE); Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:2162: LOG(ERROR) << "Input Buffer Create Error"; On 2017/06/07 10:08:12, Pawel Osciak wrote: > NOTIFY_ERROR() Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:2165: input_buffer_map_.resize(reqbufs.count); On 2017/06/07 10:08:12, Pawel Osciak wrote: > We could also use kInputBufferCount since we check that driver allocated the > exact number of buffers in Create(). Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:2507: */ On 2017/06/07 10:08:12, Pawel Osciak wrote: > On 2017/06/06 06:09:29, hiroh wrote: > > Should deallocation be executed clearly here by doing > > output_buffer_memory.reset(nullptr)? > > I would say so, yes. output_buffer_memory = nullptr; may be shorter. Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:2507: */ On 2017/06/06 06:09:29, hiroh wrote: > Should deallocation be executed clearly here by doing > output_buffer_memory.reset(nullptr)? Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:2555: */ On 2017/06/06 06:09:29, hiroh wrote: > ditto. Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_video_d... File media/gpu/v4l2_video_decode_accelerator.h (right): https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.h:583: std::unique_ptr<V4L2MmapMemory> input_buffer_memory; On 2017/06/07 10:08:12, Pawel Osciak wrote: > s/input_buffer_memory/input_buffer_memory_/ Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.h:586: std::unique_ptr<V4L2MmapMemory> output_buffer_memory; On 2017/06/07 10:08:12, Pawel Osciak wrote: > s/output_buffer_memory/output_buffer_memory_/ Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... File media/gpu/video_memory_manager.cc (right): https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.cc:43: int32_t index = mem.first; On 2017/06/07 10:08:12, Pawel Osciak wrote: > index is uint64_t? Perhaps use auto? Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.cc:56: uint64_t index = index_++; On 2017/06/07 10:08:12, Pawel Osciak wrote: > Could > https://cs.chromium.org/chromium/src/gpu/command_buffer/common/id_type.h?l=49 be > useful here? IDtype seems to be not useful in this case as far as I research. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.cc:59: base::Unretained(this), index, size); On 2017/06/07 10:08:12, Pawel Osciak wrote: > Please add a comment explaining why Unretained is safe. Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.cc:62: void VideoMemoryManager::RegisterMemoryData(uint64_t index, On 2017/06/07 10:08:12, Pawel Osciak wrote: > Perhaps we could inline this into RegisterAllocation? Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.cc:67: memory_name_[index] = name; On 2017/06/06 06:01:16, hiroh wrote: > The present implementation manages the information by a unique index instead of > the type of memory. > Both of data structures cannot have the data whose key |index|. > So, size and name are just assigned to each data structure. Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.cc:70: void VideoMemoryManager::UnregisterMemoryData(uint64_t index, size_t size) { On 2017/06/07 10:08:12, Pawel Osciak wrote: > size is unused? Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.cc:73: memory_name_.erase(index); On 2017/06/06 06:01:16, hiroh wrote: > Same as above, index is unique. memory_usage_[index] is unchanged after it is > created. > The data specified by index in both data structures are just erased. Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... File media/gpu/video_memory_manager.h (right): https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.h:14: #include "base/synchronization/lock.h" On 2017/06/07 10:08:13, Pawel Osciak wrote: > Not needed? Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.h:19: // This class trucks all the memory used in Video Codec. On 2017/06/07 10:08:13, Pawel Osciak wrote: > s/trucks/tracks/ Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.h:20: // The information of the memories dumped by this class can be shown On 2017/06/07 10:08:13, Pawel Osciak wrote: > s/memories/memory/ Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.h:24: // MMAP memory, VASurface On 2017/06/07 10:08:13, Pawel Osciak wrote: > Similarly, this could suggest no other clients could use this class. Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.h:25: // TODO(hiroh): enumerates all the managed memory On 2017/06/07 10:08:13, Pawel Osciak wrote: > Please be more specific, so others could understand what is required to do here. Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.h:27: // Classes in Video Codec do not call this class directlly. On 2017/06/07 10:08:13, Pawel Osciak wrote: > s/directlly/directly/ Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.h:30: class VideoMemoryManager : public base::trace_event::MemoryDumpProvider { On 2017/06/07 10:08:13, Pawel Osciak wrote: > Please document that all methods of this class must be called on the same > thread. Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.h:40: // Return Callback function to decrease the memory usage by |size| On 2017/06/07 10:08:13, Pawel Osciak wrote: > Please update this comment. Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.h:58: std::unordered_map<uint64_t, size_t> memory_usage_; On 2017/06/07 10:08:13, Pawel Osciak wrote: > Could we have one, common map storing a struct of size and string? That would > save us a lookup and the need to maintain consistency between two maps. Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.h:62: // index_ represensts each memory type and is uqniue to each memory On 2017/06/07 10:08:13, Pawel Osciak wrote: > s/represensts/represents/ > s/uqniue/unique/ Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.h:64: // index increases monotonically On 2017/06/07 10:08:12, Pawel Osciak wrote: > Nit: please format all comment sentences in this CL to start with a capital > letter and end with a dot. Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.h:67: // By using SequenceChecker, it is guranteed that memory_usage_, On 2017/06/07 10:08:13, Pawel Osciak wrote: > s/guranteed/guaranteed/ Done. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.h:69: SEQUENCE_CHECKER(sequence_chcker_); On 2017/06/07 10:08:13, Pawel Osciak wrote: > s/sequence_chcker_/sequence_checker_/ Done.
https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... File media/gpu/v4l2_mmap_memory.cc (right): https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:26: std::unique_ptr<V4L2MmapMemory> v4l2mm(new V4L2MmapMemory(device, reqbufs)); On 2017/06/08 23:17:41, hiroh wrote: > On 2017/06/07 10:08:11, Pawel Osciak wrote: > > Perhaps pass reqbufs only to Allocate, and store it from there if we succeed > the > > allocation? > > Yes. Therefore, a pointer should be used to pass an argument to this function. My point was, I think we don't need to pass reqbufs to the constructor, only to Allocate(). https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... File media/gpu/v4l2_mmap_memory.h (right): https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:12: #include "base/memory/ref_counted.h" On 2017/06/08 23:17:41, hiroh wrote: > On 2017/06/07 10:08:12, Pawel Osciak wrote: > > Is this used? > > scoped_refptr is defined in this header file. Acknowledged. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:13: #include "media/gpu/v4l2_device.h" On 2017/06/08 23:17:41, hiroh wrote: > On 2017/06/07 10:08:11, Pawel Osciak wrote: > > Could V4L2Device be a forward-declaration instead? > > Done. > > After removing v4l2_device.h, the compilation is failed due to OnceClosure. > Because of this, I change to include callback.h instead of callback.h Acknowledged. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:28: // A new V4L2MmapMemory is constructed with |reqbufs| and |device|. On 2017/06/08 23:17:42, hiroh wrote: > On 2017/06/07 10:08:11, Pawel Osciak wrote: > > Please specify why reqbufs is a pointer if you choose to use a pointer instead > > of const ref. > > According to chromium style guide, a pointer should be used > if it is written within the function. > I write a comment that reqbufs are written in this function. Yes, however my point was, the updated value is not used by the clients of this method. So we don't need to write to this reqbufs in the method (we can make a copy, or use the class member to which we copy this). Normally if the method is not intended to use a method argument as a return value, it should be preferred not to modify arguments. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... File media/gpu/video_memory_manager.cc (right): https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory... media/gpu/video_memory_manager.cc:56: uint64_t index = index_++; On 2017/06/08 23:17:42, hiroh wrote: > On 2017/06/07 10:08:12, Pawel Osciak wrote: > > Could > > https://cs.chromium.org/chromium/src/gpu/command_buffer/common/id_type.h?l=49 > be > > useful here? > > IDtype seems to be not useful in this case as far as I research. Acknowledged. https://codereview.chromium.org/2909533003/diff/120001/media/gpu/v4l2_mmap_me... File media/gpu/v4l2_mmap_memory.h (right): https://codereview.chromium.org/2909533003/diff/120001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:34: // less buffers than request is allocated, this returns nullptr and s/request is/requested are/ https://codereview.chromium.org/2909533003/diff/120001/media/gpu/v4l2_video_d... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2909533003/diff/120001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:374: // TODO(hiroh): the name passsed to video memory manager s/passsed/passed/ https://codereview.chromium.org/2909533003/diff/120001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:375: // is acquired from VDA::Config. s/is acquired/should be acquired/ https://codereview.chromium.org/2909533003/diff/120001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:379: NOTIFY_ERROR(PLATFORM_FAILURE); Sorry I wasn't clear. Let's keep the LOG(ERROR), but we need NOTIFY_ERROR() in addition to it. https://codereview.chromium.org/2909533003/diff/120001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:2164: NOTIFY_ERROR(PLATFORM_FAILURE); ditto https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... File media/gpu/video_memory_manager.cc (right): https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.cc:38: auto reportMemoryUsage = [pmd](const std::string& name, const size_t size) { s/reportMemoryUsage/report_memory_usage/ https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.cc:45: for (auto& mem : memory_usage_) { const auto& https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.cc:48: reportMemoryUsage(name, size); Perhaps just: reportMemoryUsage(mem.second.size, mem.second.name); Or: auto allocation_info = mem.second; reportMemoryUsage(allocation_info.size, allocation_info.name); https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.cc:60: // Register memory data Let's have DCHECK(memory_usage_.count, 0u); here. This will be optimized-out on non-Debug builds. https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.cc:69: memory_usage_.erase(index); Let's have DCHECK(memory_usage_.count, 1u); here. This will be optimized-out on non-Debug builds. https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... File media/gpu/video_memory_manager.h (right): https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.h:27: // cannot use this class. Why? https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.h:32: // All the methods are executed by the same thread. s/are executed by/of this class must be called on/ https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.h:43: // to unregister memory information (UnregisterMemoryData) Please dot at the end of sentence. https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.h:54: void UnregisterMemoryData(uint64_t index); Perhaps UnregisterAllocation() to match RegisterAllocation() ? https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.h:56: // The information of memory. s/of memory/for each tracked allocation/ https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.h:57: // |size| : the size of memory. s/size of memory/size in bytes/ https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.h:59: struct memInfo { s/memInfo/MemInfo/ but perhaps AllocationInfo, since we have "RegisterAllocation"() ? https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.h:63: // The size of used memory for Video. s/The size of used memory for Video./Memory information for all allocations, keyed by index generated by this class to allow deletion by key on UnregisterMemoryData()./ https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.h:64: std::unordered_map<uint64_t, memInfo> memory_usage_; Perhaps allocations_ to match other names? https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.h:66: // |index_| represents each memory type and is unique to each memory. Next ID to use for registering the next allocation in the memory_usage_ map. Monotonic counter generated by this class. https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.h:71: // By using SequenceChecker, it is guaranteed that memory_usage__ s/__/_/
https://codereview.chromium.org/2909533003/diff/120001/media/gpu/v4l2_mmap_me... File media/gpu/v4l2_mmap_memory.h (right): https://codereview.chromium.org/2909533003/diff/120001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:34: // less buffers than request is allocated, this returns nullptr and On 2017/06/09 00:04:29, Pawel Osciak wrote: > s/request is/requested are/ Done. https://codereview.chromium.org/2909533003/diff/120001/media/gpu/v4l2_video_d... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2909533003/diff/120001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:374: // TODO(hiroh): the name passsed to video memory manager On 2017/06/09 00:04:29, Pawel Osciak wrote: > s/passsed/passed/ Done. https://codereview.chromium.org/2909533003/diff/120001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:375: // is acquired from VDA::Config. On 2017/06/09 00:04:29, Pawel Osciak wrote: > s/is acquired/should be acquired/ Done. https://codereview.chromium.org/2909533003/diff/120001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:379: NOTIFY_ERROR(PLATFORM_FAILURE); On 2017/06/09 00:04:29, Pawel Osciak wrote: > Sorry I wasn't clear. Let's keep the LOG(ERROR), but we need NOTIFY_ERROR() in > addition to it. Done. https://codereview.chromium.org/2909533003/diff/120001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:2164: NOTIFY_ERROR(PLATFORM_FAILURE); On 2017/06/09 00:04:29, Pawel Osciak wrote: > ditto Done. https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... File media/gpu/video_memory_manager.cc (right): https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.cc:38: auto reportMemoryUsage = [pmd](const std::string& name, const size_t size) { On 2017/06/09 00:04:30, Pawel Osciak wrote: > s/reportMemoryUsage/report_memory_usage/ Done. https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.cc:45: for (auto& mem : memory_usage_) { On 2017/06/09 00:04:29, Pawel Osciak wrote: > const auto& Done. https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.cc:48: reportMemoryUsage(name, size); On 2017/06/09 00:04:29, Pawel Osciak wrote: > Perhaps just: > > reportMemoryUsage(mem.second.size, mem.second.name); > > Or: > > auto allocation_info = mem.second; > reportMemoryUsage(allocation_info.size, allocation_info.name); Done. https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.cc:60: // Register memory data On 2017/06/09 00:04:29, Pawel Osciak wrote: > Let's have DCHECK(memory_usage_.count, 0u); here. This will be optimized-out on > non-Debug builds. Done. https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.cc:69: memory_usage_.erase(index); On 2017/06/09 00:04:29, Pawel Osciak wrote: > Let's have DCHECK(memory_usage_.count, 1u); here. This will be optimized-out on > non-Debug builds. Done. https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... File media/gpu/video_memory_manager.h (right): https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.h:27: // cannot use this class. On 2017/06/09 00:04:30, Pawel Osciak wrote: > Why? Done. https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.h:32: // All the methods are executed by the same thread. On 2017/06/09 00:04:30, Pawel Osciak wrote: > s/are executed by/of this class must be called on/ Done. https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.h:43: // to unregister memory information (UnregisterMemoryData) On 2017/06/09 00:04:30, Pawel Osciak wrote: > Please dot at the end of sentence. Done. https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.h:54: void UnregisterMemoryData(uint64_t index); On 2017/06/09 00:04:30, Pawel Osciak wrote: > Perhaps UnregisterAllocation() to match RegisterAllocation() ? Done. https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.h:56: // The information of memory. On 2017/06/09 00:04:30, Pawel Osciak wrote: > s/of memory/for each tracked allocation/ Done. https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.h:57: // |size| : the size of memory. On 2017/06/09 00:04:30, Pawel Osciak wrote: > s/size of memory/size in bytes/ Done. https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.h:59: struct memInfo { On 2017/06/09 00:04:30, Pawel Osciak wrote: > s/memInfo/MemInfo/ > > but perhaps AllocationInfo, since we have "RegisterAllocation"() ? Done. https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.h:63: // The size of used memory for Video. On 2017/06/09 00:04:30, Pawel Osciak wrote: > s/The size of used memory for Video./Memory information for all allocations, > keyed by index generated by this class to allow deletion by key on > UnregisterMemoryData()./ Done. https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.h:64: std::unordered_map<uint64_t, memInfo> memory_usage_; On 2017/06/09 00:04:30, Pawel Osciak wrote: > Perhaps allocations_ to match other names? Done. https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.h:66: // |index_| represents each memory type and is unique to each memory. On 2017/06/09 00:04:30, Pawel Osciak wrote: > Next ID to use for registering the next allocation in the memory_usage_ map. > Monotonic counter generated by this class. Done. https://codereview.chromium.org/2909533003/diff/120001/media/gpu/video_memory... media/gpu/video_memory_manager.h:71: // By using SequenceChecker, it is guaranteed that memory_usage__ On 2017/06/09 00:04:30, Pawel Osciak wrote: > s/__/_/ Done.
Besides V4L2 VDA, MMAP memory are dumped also in V4L2 SVDA and V4L2 VEA. I confirm the memory are dumped correctly in V4L2 SVDA and VDA. (but not yet VEA)
Besides V4L2 VDA, MMAP memory are dumped also in V4L2 SVDA and V4L2 VEA. I confirmed the memory are dumped correctly in V4L2 SVDA and VDA. (but not yet VEA)
I upload vaapi_surface_memory.{cc, h}, which trucks vaSurface memory allocated in VA VDA/VEA. I only define and implement solely these class. They are not linked actually to VA VDA/VEA. So there is no function to dump vaSurface memory yet.
I remove vaapi_surface_memory.{h,cc} so that CL can be separated.
I will upload another patch to track vaSurface. Because the patch includes a lot of changes, we should separate from this patch. This patch will have to be reviewed and submitted CQ first.
It turns out the name of memory will be not unique in the future. The key of map that stores information of memory is changed to string, not an integer.
https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_me... File media/gpu/v4l2_mmap_memory.cc (right): https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:23: std::unique_ptr<V4L2MmapMemory> V4L2MmapMemory::Create( // static https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:25: const v4l2_requestbuffers& reqbufs, I think we should at least if (reqbufs.memory != V4L2_MEMORY_MMAP) return nullptr, but just passing count and type directly instead of the whole reqbufs struct may be more explicit and simpler, and help us avoid duplicating code for filling the reqbufs struct in other classes. We could also move QUERYBUF to this class and instead have a size(size_t buffer, size_t plane) data(size_t buffer, size_t plane) methods in this class, to avoid more duplication. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:53: LOG(ERROR) << "VIDIOC_REQBUFS allocated less buffers than request"; Perhaps we could also mention how many were allocated vs how many were requested. I would also suggest PLOG() instead of LOG(). https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:72: LOG(ERROR) << "VIDIOC_QUERYBUF is failed"; PLOG() perhaps? https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:88: LOG(ERROR) << "VIDIOC_REQBUFS for deallocation is failed"; PLOG() please https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_me... File media/gpu/v4l2_mmap_memory.h (right): https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:26: class V4L2MmapMemory { Should we make this refcounted so it could be shared across multiple users that may have access to this memory (such as e.g. video processor and a codec together)? https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:32: // |reqbufs| is a const reference here. The method definition already contains the information that reqbufs is a const&, so it's not needed here. We should rather document how reqbufs is used instead. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:41: const std::string& name); Do we need to #include <string> ? https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:52: // the allocated size. in bytes. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:61: void InitDestructionCallBack(base::OnceClosure cb); This method is private and consists of only one LoC, perhaps we could inline this instead? https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:64: // device is not destructed before this class destructor. Perhaps we should instead say that we keep a ref to the device so we could free the memory via Deallocate(). https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_slice_v... File media/gpu/v4l2_slice_video_decode_accelerator.cc (right): https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_slice_v... media/gpu/v4l2_slice_video_decode_accelerator.cc:883: // Deallocate input buffer memory Nit: perhaps let's free after clearing the info structs (after l.886), just for clarity. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_slice_v... media/gpu/v4l2_slice_video_decode_accelerator.cc:1576: if (output_buffer_memory_ == nullptr) { Please add a comment why this if-else is needed. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_slice_v... media/gpu/v4l2_slice_video_decode_accelerator.cc:1579: reqbufs.count = 0; This line is actually not needed as we memset above. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_slice_v... File media/gpu/v4l2_slice_video_decode_accelerator.h (right): https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_slice_v... media/gpu/v4l2_slice_video_decode_accelerator.h:470: // V4L2MMapMemory for input buffer. s/buffer/buffers/ https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_slice_v... media/gpu/v4l2_slice_video_decode_accelerator.h:473: // V4L2MMapMemory for output buffer. s/buffer/buffers/ https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_video_d... File media/gpu/v4l2_video_decode_accelerator.h (right): https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.h:582: // V4L2MMapMemory for input buffer. s/buffer/buffers/ https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.h:585: // V4L2MMapMemory for output buffer. ditto https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_video_e... File media/gpu/v4l2_video_encode_accelerator.h (right): https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_video_e... media/gpu/v4l2_video_encode_accelerator.h:308: // V4L2MMapMemory for output buffer. s/buffer/buffers/ https://codereview.chromium.org/2909533003/diff/280001/media/gpu/video_memory... File media/gpu/video_memory_manager.cc (right): https://codereview.chromium.org/2909533003/diff/280001/media/gpu/video_memory... media/gpu/video_memory_manager.cc:73: DCHECK(it != allocations_.end()); Perhaps also DCHECK_GE(size, it->second) https://codereview.chromium.org/2909533003/diff/280001/media/gpu/video_memory... File media/gpu/video_memory_manager.h (right): https://codereview.chromium.org/2909533003/diff/280001/media/gpu/video_memory... media/gpu/video_memory_manager.h:17: // This class tracks all the memory used in Video Codec. s/Video Codec/video codec allocated in VDA implementations/ https://codereview.chromium.org/2909533003/diff/280001/media/gpu/video_memory... media/gpu/video_memory_manager.h:20: // TODO(hiroh): Not sure which type of memory are supported and I think this TODO is not needed here. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/video_memory... media/gpu/video_memory_manager.h:25: // MMAP memory, VASurface Please update/remove this as well. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/video_memory... media/gpu/video_memory_manager.h:29: // that actually manages each kind of memory (e.g. V4L2MmapMemory). This comment feels a bit confusing. Given that this class is no longer allocating/owning any memory, but only tracks, perhaps we should rename it to VideoMemoryTracker? We would probably not need this comment then (that a "*MemoryManager" class is not really managing any memory). https://codereview.chromium.org/2909533003/diff/280001/media/gpu/video_memory... media/gpu/video_memory_manager.h:42: base::OnceClosure RegisterAllocation(size_t size, const std::string& name); Please document arguments.
I changed the name VideoMemoryManager to VideoMemoryTracker. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_me... File media/gpu/v4l2_mmap_memory.cc (right): https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:23: std::unique_ptr<V4L2MmapMemory> V4L2MmapMemory::Create( On 2017/06/19 01:51:12, Pawel Osciak wrote: > // static Done. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:25: const v4l2_requestbuffers& reqbufs, On 2017/06/19 01:51:12, Pawel Osciak wrote: > I think we should at least if (reqbufs.memory != V4L2_MEMORY_MMAP) return > nullptr, but just passing count and type directly instead of the whole reqbufs > struct may be more explicit and simpler, and help us avoid duplicating code for > filling the reqbufs struct in other classes. > We could also move QUERYBUF to this class and instead have a size(size_t buffer, > size_t plane) data(size_t buffer, size_t plane) methods in this class, to avoid > more duplication. Done. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:53: LOG(ERROR) << "VIDIOC_REQBUFS allocated less buffers than request"; On 2017/06/19 01:51:12, Pawel Osciak wrote: > Perhaps we could also mention how many were allocated vs how many were > requested. I would also suggest PLOG() instead of LOG(). Done. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:72: LOG(ERROR) << "VIDIOC_QUERYBUF is failed"; On 2017/06/19 01:51:12, Pawel Osciak wrote: > PLOG() perhaps? Done. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:88: LOG(ERROR) << "VIDIOC_REQBUFS for deallocation is failed"; On 2017/06/19 01:51:12, Pawel Osciak wrote: > PLOG() please Done. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_me... File media/gpu/v4l2_mmap_memory.h (right): https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:26: class V4L2MmapMemory { On 2017/06/19 01:51:12, Pawel Osciak wrote: > Should we make this refcounted so it could be shared across multiple users that > may have access to this memory (such as e.g. video processor and a codec > together)? Because V4L2MmapMemory is not shared, it should be unique_ptr. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:32: // |reqbufs| is a const reference here. On 2017/06/19 01:51:12, Pawel Osciak wrote: > The method definition already contains the information that reqbufs is a const&, > so it's not needed here. We should rather document how reqbufs is used instead. Done. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:41: const std::string& name); On 2017/06/19 01:51:12, Pawel Osciak wrote: > Do we need to #include <string> ? There is no need to do in this header. Because <string> is contained at least in "base/logging.h", which is included "base/memory/ref_counted.h." https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:52: // the allocated size. On 2017/06/19 01:51:12, Pawel Osciak wrote: > in bytes. Done. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:61: void InitDestructionCallBack(base::OnceClosure cb); On 2017/06/19 01:51:12, Pawel Osciak wrote: > This method is private and consists of only one LoC, perhaps we could inline > this instead? Done. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:64: // device is not destructed before this class destructor. On 2017/06/19 01:51:12, Pawel Osciak wrote: > Perhaps we should instead say that we keep a ref to the device so we could free > the memory via Deallocate(). Done. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_slice_v... File media/gpu/v4l2_slice_video_decode_accelerator.cc (right): https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_slice_v... media/gpu/v4l2_slice_video_decode_accelerator.cc:883: // Deallocate input buffer memory On 2017/06/19 01:51:12, Pawel Osciak wrote: > Nit: perhaps let's free after clearing the info structs (after l.886), just for > clarity. Done. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_slice_v... media/gpu/v4l2_slice_video_decode_accelerator.cc:1576: if (output_buffer_memory_ == nullptr) { On 2017/06/19 01:51:12, Pawel Osciak wrote: > Please add a comment why this if-else is needed. Done. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_slice_v... media/gpu/v4l2_slice_video_decode_accelerator.cc:1579: reqbufs.count = 0; On 2017/06/19 01:51:12, Pawel Osciak wrote: > This line is actually not needed as we memset above. Done. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_slice_v... File media/gpu/v4l2_slice_video_decode_accelerator.h (right): https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_slice_v... media/gpu/v4l2_slice_video_decode_accelerator.h:470: // V4L2MMapMemory for input buffer. On 2017/06/19 01:51:13, Pawel Osciak wrote: > s/buffer/buffers/ Done. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_slice_v... media/gpu/v4l2_slice_video_decode_accelerator.h:473: // V4L2MMapMemory for output buffer. On 2017/06/19 01:51:12, Pawel Osciak wrote: > s/buffer/buffers/ Done. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_video_d... File media/gpu/v4l2_video_decode_accelerator.h (right): https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.h:582: // V4L2MMapMemory for input buffer. On 2017/06/19 01:51:13, Pawel Osciak wrote: > s/buffer/buffers/ Done. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.h:585: // V4L2MMapMemory for output buffer. On 2017/06/19 01:51:13, Pawel Osciak wrote: > ditto Done. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_video_e... File media/gpu/v4l2_video_encode_accelerator.h (right): https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_video_e... media/gpu/v4l2_video_encode_accelerator.h:308: // V4L2MMapMemory for output buffer. On 2017/06/19 01:51:13, Pawel Osciak wrote: > s/buffer/buffers/ Done. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/video_memory... File media/gpu/video_memory_manager.cc (right): https://codereview.chromium.org/2909533003/diff/280001/media/gpu/video_memory... media/gpu/video_memory_manager.cc:73: DCHECK(it != allocations_.end()); On 2017/06/19 01:51:13, Pawel Osciak wrote: > Perhaps also DCHECK_GE(size, it->second) Done. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/video_memory... File media/gpu/video_memory_manager.h (right): https://codereview.chromium.org/2909533003/diff/280001/media/gpu/video_memory... media/gpu/video_memory_manager.h:17: // This class tracks all the memory used in Video Codec. On 2017/06/19 01:51:13, Pawel Osciak wrote: > s/Video Codec/video codec allocated in VDA implementations/ Done. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/video_memory... media/gpu/video_memory_manager.h:20: // TODO(hiroh): Not sure which type of memory are supported and On 2017/06/19 01:51:13, Pawel Osciak wrote: > I think this TODO is not needed here. Done. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/video_memory... media/gpu/video_memory_manager.h:25: // MMAP memory, VASurface On 2017/06/19 01:51:13, Pawel Osciak wrote: > Please update/remove this as well. Done. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/video_memory... media/gpu/video_memory_manager.h:29: // that actually manages each kind of memory (e.g. V4L2MmapMemory). On 2017/06/19 01:51:13, Pawel Osciak wrote: > This comment feels a bit confusing. Given that this class is no longer > allocating/owning any memory, but only tracks, perhaps we should rename it to > VideoMemoryTracker? > We would probably not need this comment then (that a "*MemoryManager" class is > not really managing any memory). Done. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/video_memory... media/gpu/video_memory_manager.h:42: base::OnceClosure RegisterAllocation(size_t size, const std::string& name); On 2017/06/19 01:51:13, Pawel Osciak wrote: > Please document arguments. Done.
https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_me... File media/gpu/v4l2_mmap_memory.h (right): https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:26: class V4L2MmapMemory { On 2017/06/19 07:06:28, hiroh wrote: > On 2017/06/19 01:51:12, Pawel Osciak wrote: > > Should we make this refcounted so it could be shared across multiple users > that > > may have access to this memory (such as e.g. video processor and a codec > > together)? > > Because V4L2MmapMemory is not shared, it should be unique_ptr. Acknowledged. https://codereview.chromium.org/2909533003/diff/300001/media/gpu/v4l2_mmap_me... File media/gpu/v4l2_mmap_memory.cc (right): https://codereview.chromium.org/2909533003/diff/300001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:48: std::vector<BufferData> buffer_info(count_); Since we have to call QUERYBUF in Allocate() anyway, could we cache/keep this information instead from there? https://codereview.chromium.org/2909533003/diff/300001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:49: struct v4l2_plane planes[1]; VIDEO_MAX_PLANES https://codereview.chromium.org/2909533003/diff/300001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:58: buffer.length = 1; ararysize(planes) https://codereview.chromium.org/2909533003/diff/300001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:65: void* address = Is mmap() called from here and not from Create() to save on doing the mmap in case the client of this class does not need this to be mapped (and will never call GetBufferInfo())? If so, we should still ensure we don't mmap multiple times for multiple calls to GetBufferInfo. However it may not be worth the complexity, I'd probably suggest moving everything to Allocate(). https://codereview.chromium.org/2909533003/diff/300001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:67: MAP_SHARED, buffer.m.planes[0].m.mem_offset); We may need to map more planes than 1, could we iterate over the actual number of planes please? https://codereview.chromium.org/2909533003/diff/300001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:127: v4l2_requestbuffers reqbufs; We also need to unmap if we mmapped previously. https://codereview.chromium.org/2909533003/diff/300001/media/gpu/v4l2_mmap_me... File media/gpu/v4l2_mmap_memory.h (right): https://codereview.chromium.org/2909533003/diff/300001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:38: // Deallocation is done before returning, if necessary. Please update documentation above to reflect the new method signature. https://codereview.chromium.org/2909533003/diff/300001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:53: std::vector<BufferData> GetBufferInfo(); Perhaps we could make this method const? https://codereview.chromium.org/2909533003/diff/300001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:62: uint32_t type); v4l2_buf_type https://codereview.chromium.org/2909533003/diff/300001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:81: const uint32_t type_; v4l2_buf_type
VideoMemoryTracker cannot track any memory in the below situation. 1) VDA(VEA) and memory-trace turn on. 2) memory-infra stops(complete). (memory can be tracked correctly at that time.) 3) VDA turns off. 4) VDA and memory-trace restart. 5) Any video memory cannot be tracked with a lot of the following logs. [2523:2809:0620/110238.812791:ERROR:memory_dump_manager.cc(624)] Disabling MemoryDumpProvider "VideoMemoryTracker". Failed to post task on the task runner provided. I confirmed VideoMemoryTracker was not destructed. https://codereview.chromium.org/2909533003/diff/300001/media/gpu/v4l2_mmap_me... File media/gpu/v4l2_mmap_memory.cc (right): https://codereview.chromium.org/2909533003/diff/300001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:48: std::vector<BufferData> buffer_info(count_); On 2017/06/19 09:44:32, Pawel Osciak wrote: > Since we have to call QUERYBUF in Allocate() anyway, could we cache/keep this > information instead from there? Done. https://codereview.chromium.org/2909533003/diff/300001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:49: struct v4l2_plane planes[1]; On 2017/06/19 09:44:32, Pawel Osciak wrote: > VIDEO_MAX_PLANES Done. https://codereview.chromium.org/2909533003/diff/300001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:58: buffer.length = 1; On 2017/06/19 09:44:32, Pawel Osciak wrote: > ararysize(planes) Done. https://codereview.chromium.org/2909533003/diff/300001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:65: void* address = On 2017/06/19 09:44:32, Pawel Osciak wrote: > Is mmap() called from here and not from Create() to save on doing the mmap in > case the client of this class does not need this to be mapped (and will never > call GetBufferInfo())? > > If so, we should still ensure we don't mmap multiple times for multiple calls to > GetBufferInfo. > > However it may not be worth the complexity, I'd probably suggest moving > everything to Allocate(). Done. https://codereview.chromium.org/2909533003/diff/300001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:67: MAP_SHARED, buffer.m.planes[0].m.mem_offset); On 2017/06/19 09:44:32, Pawel Osciak wrote: > We may need to map more planes than 1, could we iterate over the actual number > of planes please? Done. https://codereview.chromium.org/2909533003/diff/300001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:127: v4l2_requestbuffers reqbufs; On 2017/06/19 09:44:32, Pawel Osciak wrote: > We also need to unmap if we mmapped previously. Done. https://codereview.chromium.org/2909533003/diff/300001/media/gpu/v4l2_mmap_me... File media/gpu/v4l2_mmap_memory.h (right): https://codereview.chromium.org/2909533003/diff/300001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:38: // Deallocation is done before returning, if necessary. On 2017/06/19 09:44:32, Pawel Osciak wrote: > Please update documentation above to reflect the new method signature. Done. https://codereview.chromium.org/2909533003/diff/300001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:53: std::vector<BufferData> GetBufferInfo(); On 2017/06/19 09:44:32, Pawel Osciak wrote: > Perhaps we could make this method const? Done. https://codereview.chromium.org/2909533003/diff/300001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:62: uint32_t type); On 2017/06/19 09:44:32, Pawel Osciak wrote: > v4l2_buf_type Done. https://codereview.chromium.org/2909533003/diff/300001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:81: const uint32_t type_; On 2017/06/19 09:44:32, Pawel Osciak wrote: > v4l2_buf_type Done.
https://codereview.chromium.org/2909533003/diff/300001/media/gpu/v4l2_mmap_me... File media/gpu/v4l2_mmap_memory.h (right): https://codereview.chromium.org/2909533003/diff/300001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:69: size_t Allocate(); why can't the ctor allocate memory and dtor free it? https://codereview.chromium.org/2909533003/diff/300001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:85: base::OnceClosure destruction_cb_; why is a callback needed? if the allocator and memory tracker is one and the same then there's no need for this. https://codereview.chromium.org/2909533003/diff/300001/media/gpu/video_memory... File media/gpu/video_memory_tracker.cc (right): https://codereview.chromium.org/2909533003/diff/300001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:26: base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider( This doesn't seem like the appropriate place to call this. This is a singleton and it's not well-defined when and on what thread this is called. https://codereview.chromium.org/2909533003/diff/300001/media/gpu/video_memory... File media/gpu/video_memory_tracker.h (right): https://codereview.chromium.org/2909533003/diff/300001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:17: // This class tracks all the memory used in video codec allocated in why does allocation need to be separated from tracking? the normal behavior is to have the allocator track memory usage as it typically has to do that anyhow and adding a separate tracker is just duplicating code and adding unnecessary code and bugs to maintain.. https://codereview.chromium.org/2909533003/diff/300001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:26: static VideoMemoryTracker* GetInstance(); Does this have to be a singleton at this level? Can the user of this instead decide the lifetime and what thread this can be used from?
I fixed, so that VideoMemoryTracker can dump even in the abovementioned situation.
hiroh@chromium.org changed reviewers: + sadrul@chromium.org
I added sadurl@ to owner to ask my change to gpu/ipc/service/{DEPS, gpu_init.cc} is OK. VideoMemoryTracker dumps the memory used in video codec allocated VideoDecodeAcclerator and VideoEncodeAccelerator. For VideoMemoryTracker to work correctly, there is need to push its lifetime decision and the registration of the MemoryDumpProvider to a higher level. I decided putting it gpu_init.cc this time. Including media/gpu/video_memory_tracker.h violates checkdeps rules. So, I had to change DEPS in gpu/ipc/service. Sadurl, can you review the change in gpu/ipc/service? Thanks.
My goal is to track all important memory used in video codec allocated for Video Decode (Encode) Acceleration. There are several kind of memory, such as, MMAP memory allocated in V4L2 (S)VDA/VEA and vaSurface memory allocated in VA VDA/VEA. The summary of present design is as follows: There is only one MemoryDumpProvider, that is VideoMemoryTracker. It manages the information (e.g. name and size) of all such memory. There is also a class for each memory (e.g. V4L2MmapMemory and vaSurface(crrev.com/2944583002)). The class has a responsibility of the lifetime of specified memory, and report increase (decrease) of the size of memory when actually allocating (deallocating) the memory via VideoMemoryTracker's functions. I think this is a better design, comparing that each memory class is MDP and tracks and manages the specified memory. Because we can add the new memory class easily and almost functions related to MDP (e.g. OnMemoryDump) can be similar , then should be shared.
reveman@chromium.org changed reviewers: + primiano@chromium.org
https://codereview.chromium.org/2909533003/diff/340001/media/gpu/video_memory... File media/gpu/video_memory_tracker.cc (right): https://codereview.chromium.org/2909533003/diff/340001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:27: base::trace_event::MemoryDumpManager::GetInstance()->UnregisterDumpProvider( I think for sanity the same code that register this MDP should unregister it https://codereview.chromium.org/2909533003/diff/340001/media/gpu/video_memory... File media/gpu/video_memory_tracker.h (right): https://codereview.chromium.org/2909533003/diff/340001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:21: // Any class can track their own memory by using this class. What's the benefit of having this class instead of having the callers of IncreaseAllocation/DecreaseAllocation track their own memory? If this is actually better, then why is memory-infra in general not using one MemoryTracker instance instead of having everyone implement the MDP interface? This patch seems to conflict with other memory-infra code by adding another level of abstraction that is not needed anywhere else. +primiano for feedback. https://codereview.chromium.org/2909533003/diff/340001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:26: static VideoMemoryTracker* GetInstance(); Why still force singleton pattern here? Can't this instance live in GpuInit instead?
https://codereview.chromium.org/2909533003/diff/340001/gpu/ipc/service/DEPS File gpu/ipc/service/DEPS (right): https://codereview.chromium.org/2909533003/diff/340001/gpu/ipc/service/DEPS#n... gpu/ipc/service/DEPS:2: "+media/gpu", I see code in media/gpu depend on code in gpu/ipc/service. So this looks like a circular dependency?
Some comments inline below. By quickly glancing at the code I generally agree with the feeling that there seem to be more layers that necessary here (but I really know very little about this part of the codebase) https://codereview.chromium.org/2909533003/diff/340001/media/gpu/video_memory... File media/gpu/video_memory_tracker.cc (right): https://codereview.chromium.org/2909533003/diff/340001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:27: base::trace_event::MemoryDumpManager::GetInstance()->UnregisterDumpProvider( On 2017/06/21 15:10:49, reveman wrote: > I think for sanity the same code that register this MDP should unregister it +1. Also if this thing is a singleton why bothering unregistering that at all? https://codereview.chromium.org/2909533003/diff/340001/media/gpu/video_memory... File media/gpu/video_memory_tracker.h (right): https://codereview.chromium.org/2909533003/diff/340001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:21: // Any class can track their own memory by using this class. On 2017/06/21 15:10:49, reveman wrote: > What's the benefit of having this class instead of having the callers of > IncreaseAllocation/DecreaseAllocation track their own memory? > > If this is actually better, then why is memory-infra in general not using one > MemoryTracker instance instead of having everyone implement the MDP interface? > This patch seems to conflict with other memory-infra code by adding another > level of abstraction that is not needed anywhere else. +primiano for feedback. So this pattern could make sense if you had something like hundred (or even just a dozen) places where this memory is managed and instead of distributing the bookkeeping responsibility and ask dozen classes to keep the counts you had that centralized in a class like this. However, by looking at this CL, there seems to be only one client of this class (v4l2_mmap_memory.cc). In which case I agree with reveman's objection: why isn't v4l2_mmap_memory.cc directly implementing MDP? How many instances of that you have around? https://codereview.chromium.org/2909533003/diff/340001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:35: base::OnceClosure RegisterAllocation(size_t size, const std::string& name); I am not an owner here, but returning a OnceClosure just to materialize a call to "Unregister" seems IMHO overkill and unnecessary pain for whoever next will have to debug this class.
I commented why I adopt this design. https://codereview.chromium.org/2909533003/diff/340001/gpu/ipc/service/DEPS File gpu/ipc/service/DEPS (right): https://codereview.chromium.org/2909533003/diff/340001/gpu/ipc/service/DEPS#n... gpu/ipc/service/DEPS:2: "+media/gpu", On 2017/06/21 21:15:34, sadrul wrote: > I see code in media/gpu depend on code in gpu/ipc/service. So this looks like a > circular dependency? I decide moving "RegisterDumpProvider" to another place in order to get rid of circular dependency. Then, there is no change by this CL in gpu/ipc/service. https://codereview.chromium.org/2909533003/diff/340001/media/gpu/video_memory... File media/gpu/video_memory_tracker.cc (right): https://codereview.chromium.org/2909533003/diff/340001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:27: base::trace_event::MemoryDumpManager::GetInstance()->UnregisterDumpProvider( On 2017/06/21 15:10:49, reveman wrote: > I think for sanity the same code that register this MDP should unregister it I change as executing RegisterDumpProvider in VideoMemoryTracker's constructor. I also remove UnregisterDumpProvider because this instance is singleton, as Primiano said. https://codereview.chromium.org/2909533003/diff/340001/media/gpu/video_memory... File media/gpu/video_memory_tracker.h (right): https://codereview.chromium.org/2909533003/diff/340001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:21: // Any class can track their own memory by using this class. On 2017/06/21 15:10:49, reveman wrote: > What's the benefit of having this class instead of having the callers of > IncreaseAllocation/DecreaseAllocation track their own memory? > > If this is actually better, then why is memory-infra in general not using one > MemoryTracker instance instead of having everyone implement the MDP interface? > This patch seems to conflict with other memory-infra code by adding another > level of abstraction that is not needed anywhere else. +primiano for feedback. There are three reasons. First, the same kind of memory (MMAP memory allocated by system call) are used in multiple VDA/VEAs. Creating the memory class (e.g. V4L2MMAP memory), we can share the code. In addition, by separating MDP and each memory class, it is possible to share the code such as OnMemoryDump. Second, there are several kind of memory to be tracked. For example, vaSurface in VA VDA/VEA are going to be tracked, similar to V4L2MmapMemory. (See crrev.com/2944583002) Finally third, if a memory class is MDP, it could not manage the lifetime of a memory. If the class is designed so, MDP is created whenever a memory is allocated. https://codereview.chromium.org/2909533003/diff/340001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:26: static VideoMemoryTracker* GetInstance(); On 2017/06/21 15:10:49, reveman wrote: > Why still force singleton pattern here? Can't this instance live in GpuInit > instead? I moved RegisterDumpProvider to this class' constructor. To guarantee it is called just once, there are two solutions, singleton pattern and GpuInit. If putting this instance in GpuInit, circular dependency may cause. Therefore, we put this instance in media/gpu/gpu_video_{decode/encode}_accelerator.cc. Because these can be called multiple times, VideoMemoryTracker can be created and destructed multiple times. In this case, it is useful to use singleton pattern, which is created an instance globally once. Hence, I adopt singleton pattern. https://codereview.chromium.org/2909533003/diff/340001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:35: base::OnceClosure RegisterAllocation(size_t size, const std::string& name); On 2017/06/22 07:39:38, Primiano Tucci wrote: > I am not an owner here, but returning a OnceClosure just to materialize a call > to "Unregister" seems IMHO overkill and unnecessary pain for whoever next will > have to debug this class. Done.
Minor update to fix a future bug and DCHECK().
Rebase. V4L2MmapMemory is used in V4L2 JDA (I previously forgot that). Task runner mechanism is used in VideoMemoryTracker instead of sequence checker. A bug in VideoMemoryTrakcer is also fixed. primiano@, can you approve this MemoryDumpProvider style? posiak@ discussed with reveman@ last week and @reveman approves this style now.
Minor bug is fixed around VideoMemoryTracker::DecreaseAllocation
sadrul@chromium.org changed reviewers: - sadrul@chromium.org
Removing myself from reviewer list since changes in gpu/ipc/service have gone away.
The overall design makes more sense (specifically making the VMT a singleton). However, as-is, this CL has a clear UAF in OnMemoryDump because arbitrarily extends the lifetime of an Unretained pointer beyond its call stack. On the good side, that should be quite easy to fix. See comments below. https://codereview.chromium.org/2909533003/diff/440001/media/gpu/video_memory... File media/gpu/video_memory_tracker.cc (right): https://codereview.chromium.org/2909533003/diff/440001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:38: tracker_task_runner_->PostTask( You can't PostTask the OnMemoryDump call, there are some issues here;: 1) from a pragmatic viepoint, the |pmd| will no longer be valid after this function returns. You cannot pass and use it around. This is not memory-infra specific, it's a general pattern in chrome. If you are passed a Pointer* as argument you cannot assume that its lifetime will be valid after the function return. As such you should not pass it around. * 2) from a design viewpoint here what I see is: - You are registering the VMT with memory-infra asking to dump on the thread where the ctor is called - but then you want to PostTask the call to the tracker_thread_ So, proposal: why don't you pass tracker_thread_ (instead of ThreadTaskRunnerHandle::Get()) as arugment to RegisterDumpProvider above? * I am surprised that this code builds honestly. I though that base::Bind was forcing ptr arugments to be explicitly Unretained(), which makes evident that you are extending the lifetime. https://codereview.chromium.org/2909533003/diff/440001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:59: report_allocation(size, name); in all honestly I don't see a need for the lambda above. Why don't you just move that code inside here? https://codereview.chromium.org/2909533003/diff/440001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:63: base::Closure VideoMemoryTracker::RegisterAllocation(size_t size, I see what you are doing here, you want to create a RAII object, so the caller doesn't forget to unregister this. Conceptually make sense, but the pattern is a bit odd. You are now moving the problem to "remember to invoke this closure in your dtor". I think what you really want instead is an actual RAII pattern, i.e. a move-only object that does this PostTask when it gets destroyed, so the caller has no way to "forget" about this. What I would do is something like this: class VideoMemoryTrackerHandle { VideoMemoryTrackerHandle(size, name); ~VideoMemoryTrackerHandle() { VideoMemoryTracker::DecreaseAllocation(size,name) } DISALLOW_COPY_AND_ASSIGN(...); } and here I'd return a unique_ptr<VideoMemoryTrackerHandle> https://codereview.chromium.org/2909533003/diff/440001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:67: return base::Bind(&VideoMemoryTracker::DecreaseAllocation, just checking is it intentional that "Register" calls a "Decrease"? https://codereview.chromium.org/2909533003/diff/440001/media/gpu/video_memory... File media/gpu/video_memory_tracker.h (right): https://codereview.chromium.org/2909533003/diff/440001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:62: // Tracker task runner. nit: add a \n
Modify VideoMemoryTracker's design, referring primiano@'s review. https://codereview.chromium.org/2909533003/diff/440001/media/gpu/video_memory... File media/gpu/video_memory_tracker.cc (right): https://codereview.chromium.org/2909533003/diff/440001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:38: tracker_task_runner_->PostTask( On 2017/07/07 08:51:17, Primiano Tucci wrote: > You can't PostTask the OnMemoryDump call, there are some issues here;: > 1) from a pragmatic viepoint, the |pmd| will no longer be valid after this > function returns. You cannot pass and use it around. This is not memory-infra > specific, it's a general pattern in chrome. If you are passed a Pointer* as > argument you cannot assume that its lifetime will be valid after the function > return. As such you should not pass it around. * > > 2) from a design viewpoint here what I see is: > - You are registering the VMT with memory-infra asking to dump on the thread > where the ctor is called > - but then you want to PostTask the call to the tracker_thread_ > > So, proposal: why don't you pass tracker_thread_ (instead of > ThreadTaskRunnerHandle::Get()) as arugment to RegisterDumpProvider above? > > > * I am surprised that this code builds honestly. I though that base::Bind was > forcing ptr arugments to be explicitly Unretained(), which makes evident that > you are extending the lifetime. Done. https://codereview.chromium.org/2909533003/diff/440001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:59: report_allocation(size, name); On 2017/07/07 08:51:17, Primiano Tucci wrote: > in all honestly I don't see a need for the lambda above. Why don't you just move > that code inside here? Done. https://codereview.chromium.org/2909533003/diff/440001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:63: base::Closure VideoMemoryTracker::RegisterAllocation(size_t size, On 2017/07/07 08:51:17, Primiano Tucci wrote: > I see what you are doing here, you want to create a RAII object, so the caller > doesn't forget to unregister this. > Conceptually make sense, but the pattern is a bit odd. You are now moving the > problem to "remember to invoke this closure in your dtor". > I think what you really want instead is an actual RAII pattern, i.e. a move-only > object that does this PostTask when it gets destroyed, so the caller has no way > to "forget" about this. > > What I would do is something like this: > > class VideoMemoryTrackerHandle { > VideoMemoryTrackerHandle(size, name); > ~VideoMemoryTrackerHandle() { > VideoMemoryTracker::DecreaseAllocation(size,name) > } > > DISALLOW_COPY_AND_ASSIGN(...); > } > > and here I'd return a unique_ptr<VideoMemoryTrackerHandle> Done. https://codereview.chromium.org/2909533003/diff/440001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:67: return base::Bind(&VideoMemoryTracker::DecreaseAllocation, On 2017/07/07 08:51:17, Primiano Tucci wrote: > just checking is it intentional that "Register" calls a "Decrease"? Done. https://codereview.chromium.org/2909533003/diff/440001/media/gpu/video_memory... File media/gpu/video_memory_tracker.h (right): https://codereview.chromium.org/2909533003/diff/440001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:62: // Tracker task runner. On 2017/07/07 08:51:17, Primiano Tucci wrote: > nit: add a \n Done.
The tracker looks good now. My only concern is that starting a brand new thread just to postTask for the tracker seems really overkill. Chromium is moving toward reducing number of threads instead. Sicne it looks like you don't care about which specific thread, as long as it's one, can you just use the current thread (no idea which one is it)? Or is that not guaranteed to live long enough? If really you don't have options as a last resort I'd use base::TaskScheduler::CreateSequencedTaskRunnerWithTraits to obtain a cheap sequenced task runner. But before going there I'd try to make sure that none of the existing threads are suitable. https://codereview.chromium.org/2909533003/diff/460001/media/gpu/video_memory... File media/gpu/video_memory_tracker.cc (right): https://codereview.chromium.org/2909533003/diff/460001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:24: : tracker_thread_("VideoMemoryTrackerThread") { can you use an existing thread? Starting a new thread just for posting these tasks seems quite overkill. https://codereview.chromium.org/2909533003/diff/460001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:59: std::unique_ptr<VideoMemoryTrackerHandle> vmh( nit: auto vmh = base::MakeUnique<VideoMemoryTrackerHandle>(size, name);
I set VideoMemoryTracker's task runner is one of GPU main thread. https://codereview.chromium.org/2909533003/diff/460001/media/gpu/video_memory... File media/gpu/video_memory_tracker.cc (right): https://codereview.chromium.org/2909533003/diff/460001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:24: : tracker_thread_("VideoMemoryTrackerThread") { On 2017/07/11 09:42:59, Primiano Tucci wrote: > can you use an existing thread? Starting a new thread just for posting these > tasks seems quite overkill. Done. https://codereview.chromium.org/2909533003/diff/460001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:59: std::unique_ptr<VideoMemoryTrackerHandle> vmh( On 2017/07/11 09:42:59, Primiano Tucci wrote: > nit: auto vmh = base::MakeUnique<VideoMemoryTrackerHandle>(size, name); Done.
rebase
/media/gpu/video_memory_tracker.* LGTM with some final comments. That seems the only file that interacts with memory-infra. If there is something else you want me to look at let me know https://codereview.chromium.org/2909533003/diff/500001/media/gpu/video_memory... File media/gpu/video_memory_tracker.cc (right): https://codereview.chromium.org/2909533003/diff/500001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:43: base::StringPrintf("Video Memory/%s", name.c_str())); small nit, the other columns are named lower_with_under, can you rename this to video_memory/%s ? I realzied only now sorry https://codereview.chromium.org/2909533003/diff/500001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:53: return size ? base::MakeUnique<VideoMemoryTrackerHandle>(size, name) don't add other if statements if you have a DCHECK already, see https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... "A consequence of this is that you should not handle DCHECK() failures, even if failure would result in a crash. Attempting to handle a DCHECK() failure is a statement that the DCHECK() can fail" https://codereview.chromium.org/2909533003/diff/500001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:59: DCHECK(tracker_task_runner_); no need for this DCHECK, if this is null the next statement will crash anyways. https://codereview.chromium.org/2909533003/diff/500001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:72: DCHECK(tracker_task_runner_); ditto https://codereview.chromium.org/2909533003/diff/500001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:80: auto it = allocations_.find(name); here I think you just want to return rather than DCHECK if it == allocations_.end https://codereview.chromium.org/2909533003/diff/500001/media/gpu/video_memory... File media/gpu/video_memory_tracker.h (right): https://codereview.chromium.org/2909533003/diff/500001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:8: #include <unordered_map> See brettw's PSA in [chromium-dev] Please avoid std::unordered_map https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/brett$... use a map instead?
Update! https://codereview.chromium.org/2909533003/diff/500001/media/gpu/video_memory... File media/gpu/video_memory_tracker.cc (right): https://codereview.chromium.org/2909533003/diff/500001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:43: base::StringPrintf("Video Memory/%s", name.c_str())); On 2017/07/12 15:04:03, Primiano Tucci wrote: > small nit, the other columns are named lower_with_under, can you rename this to > video_memory/%s ? > I realzied only now sorry Done. https://codereview.chromium.org/2909533003/diff/500001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:53: return size ? base::MakeUnique<VideoMemoryTrackerHandle>(size, name) On 2017/07/12 15:04:03, Primiano Tucci wrote: > don't add other if statements if you have a DCHECK already, see > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > "A consequence of this is that you should not handle DCHECK() failures, even if > failure would result in a crash. Attempting to handle a DCHECK() failure is a > statement that the DCHECK() can fail" This DCHECK only check size >=0. The if statement in return is checking whether size is zero or not. Therefore, it does not violate DCHECK rules. https://codereview.chromium.org/2909533003/diff/500001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:59: DCHECK(tracker_task_runner_); On 2017/07/12 15:04:03, Primiano Tucci wrote: > no need for this DCHECK, if this is null the next statement will crash anyways. Done. https://codereview.chromium.org/2909533003/diff/500001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:72: DCHECK(tracker_task_runner_); On 2017/07/12 15:04:03, Primiano Tucci wrote: > ditto Done. https://codereview.chromium.org/2909533003/diff/500001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:80: auto it = allocations_.find(name); On 2017/07/12 15:04:03, Primiano Tucci wrote: > here I think you just want to return rather than DCHECK if it == > allocations_.end Done. https://codereview.chromium.org/2909533003/diff/500001/media/gpu/video_memory... File media/gpu/video_memory_tracker.h (right): https://codereview.chromium.org/2909533003/diff/500001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:8: #include <unordered_map> On 2017/07/12 15:04:03, Primiano Tucci wrote: > See brettw's PSA in > > [chromium-dev] Please avoid std::unordered_map > > https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/brett$... > > use a map instead? Done.
Description was changed from ========== Support Memory-infra for HW video codec allocations on CrOS Show driver-allocated memory (e.g. MMAP memory and VASurface). BUG=chromium:721674 TEST=seeing memory-infra CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Support Memory-infra for MMAP memory allocated in V4L2 (S)VDA/VEA on CrOS Track and dump driver-allocated MMAP memory in V4L2 (S)VDA/VEA. Show that memory usage in chrome://tracing (memory-infra). BUG=chromium:721674 TEST=seeing chrome://tracing (memory-infra) CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was checked by hiroh@chromium.org to run a CQ dry run
The CQ bit was unchecked by hiroh@chromium.org
The CQ bit was checked by hiroh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
https://codereview.chromium.org/2909533003/diff/520001/media/gpu/gpu_video_de... File media/gpu/gpu_video_decode_accelerator_factory.cc (right): https://codereview.chromium.org/2909533003/diff/520001/media/gpu/gpu_video_de... media/gpu/gpu_video_decode_accelerator_factory.cc:8: #include "base/trace_event/memory_dump_manager.h" Is this include needed? https://codereview.chromium.org/2909533003/diff/520001/media/gpu/gpu_video_en... File media/gpu/gpu_video_encode_accelerator_factory.cc (right): https://codereview.chromium.org/2909533003/diff/520001/media/gpu/gpu_video_en... media/gpu/gpu_video_encode_accelerator_factory.cc:112: VideoMemoryTracker::GetInstance(); Could this be outside of the loop? https://codereview.chromium.org/2909533003/diff/520001/media/gpu/gpu_video_en... File media/gpu/gpu_video_encode_accelerator_factory.h (right): https://codereview.chromium.org/2909533003/diff/520001/media/gpu/gpu_video_en... media/gpu/gpu_video_encode_accelerator_factory.h:12: #include "media/gpu/video_memory_tracker.h" Could this include go into .cc file instead? https://codereview.chromium.org/2909533003/diff/520001/media/gpu/ipc/service/... File media/gpu/ipc/service/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/2909533003/diff/520001/media/gpu/ipc/service/... media/gpu/ipc/service/gpu_jpeg_decode_accelerator.cc:333: VideoMemoryTracker::GetInstance(); Could this be outside of the loop? https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_jpeg_de... File media/gpu/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:370: // TODO(hiroh): The name passed to VideoMemoryTracker Please file an issue for this and link to it here. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:376: if (input_buffer_memory_ == nullptr) { if (!input_buffer_memory_) https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:387: LOG(ERROR) << "getting buffer information failed"; Do we need to PostNotifyError? Here and below. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:398: input_buffer_map_[i].address[j] = buffer_info[i][j].address; Perhaps just store BufferData directly in the map? https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... File media/gpu/v4l2_mmap_memory.cc (right): https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:17: uint32_t count, count argument should not be needed here. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:41: video_mem_tracker->RegisterAllocation(allocated_size, name); Should we verify the return value? https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... File media/gpu/v4l2_mmap_memory.h (right): https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:11: #include "base/callback.h" Is this header needed? https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:20: // This class must be created only through Create(). This comment is probably not needed, since the code indicates this by making constructors private. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:26: class V4L2MmapMemory { Is this class thread safe? Do we need a sequence checker? https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:28: // Return unique_ptr of new V4L2MmapMemory. This sentence is not needed. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:35: // chrome://tracing (memory-infra). We should mention that this both allocates and maps the memory. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:37: // * reqbufs.memory is not V4L2_MEMORY_MMAP. This is not applicable here anymore. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:55: // Because buffer_info_ is initialized previously in Allocate(), This sentence is not needed. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:62: DISALLOW_IMPLICIT_CONSTRUCTORS(V4L2MmapMemory); Please place this at the end of the private section. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:66: uint32_t v4l2_buf_type); s/uint32_t/enum v4l2_buf_type/ https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:72: // Before calling Allocate, device_, type_ and count_ must be initialized This sentence is not needed; we can't call Allocate() if the constructor did not execute. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:86: const uint32_t count_; count_ does not need to be stored. Please pass count directly to Allocate() from Create(). https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:87: const uint32_t v4l2_buf_type_; enum v4l2_buf_type https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:94: std::unique_ptr<VideoMemoryTrackerHandle> vm_hundle_; s/hundle/handle/ https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_slice_v... File media/gpu/v4l2_slice_video_decode_accelerator.cc (right): https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_slice_v... media/gpu/v4l2_slice_video_decode_accelerator.cc:762: // should be acquired from VDA::Config. Please add an issue. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_slice_v... media/gpu/v4l2_slice_video_decode_accelerator.cc:776: LOG(ERROR) << "getting buffer information failed"; NOTIFY_ERROR? Here and below. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_slice_v... media/gpu/v4l2_slice_video_decode_accelerator.cc:781: if (buffer_info[i].size() != 1) { input_planes_count_? https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_slice_v... media/gpu/v4l2_slice_video_decode_accelerator.cc:782: VLOG(1) << "The number of allocated planes is not 1."; Is this not a critical error? https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_slice_v... media/gpu/v4l2_slice_video_decode_accelerator.cc:785: input_buffer_map_[i].address = buffer_info[i][0].address; Could we just store BufferData? https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_slice_v... media/gpu/v4l2_slice_video_decode_accelerator.cc:1565: // the output buffers should be deallocated by the usual method. should be deallocated explicitly here, instead of being released via the V4L2MmapMemory. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_slice_v... media/gpu/v4l2_slice_video_decode_accelerator.cc:1566: // To avoid deallocating twice, the if context is necessary here. This sentence is not needed. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_slice_v... media/gpu/v4l2_slice_video_decode_accelerator.cc:1612: output_buffer_memory_ = V4L2MmapMemory::Create( This would result in mapping allocated memory into the userspace via mmap(), but we don't use that and should not be doing that. These buffers may not always be mappable and they may be very big, so this could consume considerable amounts of virtual memory address space. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_video_d... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:371: output_buffer_memory_ = V4L2MmapMemory::Create( The previous comment in V4L2SVDA also applies here. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:2167: return false; NOTIFY_ERROR? https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:2174: input_buffer_map_[i].address = buffer_info[i][0].address; Similar comments as in V4L2SVDA. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_video_e... File media/gpu/v4l2_video_encode_accelerator.cc (right): https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_video_e... media/gpu/v4l2_video_encode_accelerator.cc:1260: // TODO(hiroh): The name passed to VideoMemoryTracker Please add an issue number. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_video_e... media/gpu/v4l2_video_encode_accelerator.cc:1283: output_buffer_map_[i].address = buffer_info[i][0].address; Could we store BufferData? https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... File media/gpu/video_memory_tracker.cc (right): https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:7: #include "base/callback.h" Is this used? https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:36: DCHECK(tracker_task_runner_->BelongsToCurrentThread()); Could sequencechecker be used for this? https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:40: auto size = mem.second; const auto? https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:57: void VideoMemoryTracker::IncreaseAllocation(const size_t size, Perhaps inline this in RegisterAllocation? https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:65: DCHECK(tracker_task_runner_->BelongsToCurrentThread()); This DCHECK would always be true given the above if() https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:77: DCHECK(tracker_task_runner_->BelongsToCurrentThread()); ditto https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:83: DCHECK_GE(it->second, size); This should probably be an if https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... File media/gpu/video_memory_tracker.h (right): https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:11: #include "base/memory/singleton.h" Do we need to include singleton.h in .h file? Perhaps we could do this only in .cc. Please see https://cs.chromium.org/chromium/src/base/memory/singleton.h?rcl=e9e0a9019bcf.... https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:12: #include "base/threading/thread.h" We should be able to only just declare SingleThreadTaskRunner? https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:20: // VDA and VEA implementations. This could also be JDA. But I don't think anything about this class is specific to VDA/VEA/JDA, so maybe it's better to say it gathers and tracks video codec memory allocations done by its clients, especially since you say below "Any class can track their own memory by using this class." https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:30: // MemoryDumpProvider implementation Nit: please dots at the end of sentences. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:34: // Register memory information that the memory whose name is |name| is Add |size| bytes to the existing entry of |name| on allocation list, or add a new one if none such exists. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:42: void IncreaseAllocation(const size_t size, const std::string& name); Could we do this in VMT::RegisterAllocation and make this private? https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:59: // Tracker task runner. This is a task runner of a GPU main thread. Do we need this tracker to run on GPUMain? It would be preferable to not block GPUMain if not needed. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:67: class VideoMemoryTrackerHandle { Could this be a subclass of VMT? https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:69: VideoMemoryTrackerHandle(const size_t size, const std::string& name) Perhaps instead pass a closure with size and name bound to it, and just call it in destructor? DecreaseMemorySize wouldn't have to be public either then.
The CQ bit was checked by hiroh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I have one unresolved comment. On 2017/07/14 07:17:35, Pawel Osciak wrote: > Do we need this tracker to run on GPUMain? It would be preferable to not block > GPUMain if not needed. I understood we retain from posting tasks to GPU thread. Is there such a long life thread except GPU main thread as following conditions: * it is alive until Chrome runs. * tasks can be posted to it and cannot affect chrome performance. Yeah. Finding a thread like this is a part of my task. I am investigating now. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/gpu_video_de... File media/gpu/gpu_video_decode_accelerator_factory.cc (right): https://codereview.chromium.org/2909533003/diff/520001/media/gpu/gpu_video_de... media/gpu/gpu_video_decode_accelerator_factory.cc:8: #include "base/trace_event/memory_dump_manager.h" On 2017/07/14 07:17:32, Pawel Osciak wrote: > Is this include needed? This is needed. video_memory_tracker.h and video_memory_dumper.h don't include memory_dump_manager.h. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/gpu_video_en... File media/gpu/gpu_video_encode_accelerator_factory.cc (right): https://codereview.chromium.org/2909533003/diff/520001/media/gpu/gpu_video_en... media/gpu/gpu_video_encode_accelerator_factory.cc:112: VideoMemoryTracker::GetInstance(); On 2017/07/14 07:17:32, Pawel Osciak wrote: > Could this be outside of the loop? We can do it. But is it better to put within the loop in order not to call GetInstance() until VEA is actually created?? Then, should I put it the previous line of "return vea"? I don't do so since I considered VideoMemoryTracker is needed in some VEA initialize function. Ate least, it is not reasonable that VideoMemoryTracker cannot be initialized in an initialize function. Therefore, a yet another VEA class developer will accidentally do it, which will cause a bug around the lifetime of VideoMemoryTracker. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/gpu_video_en... File media/gpu/gpu_video_encode_accelerator_factory.h (right): https://codereview.chromium.org/2909533003/diff/520001/media/gpu/gpu_video_en... media/gpu/gpu_video_encode_accelerator_factory.h:12: #include "media/gpu/video_memory_tracker.h" On 2017/07/14 07:17:32, Pawel Osciak wrote: > Could this include go into .cc file instead? Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/ipc/service/... File media/gpu/ipc/service/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/2909533003/diff/520001/media/gpu/ipc/service/... media/gpu/ipc/service/gpu_jpeg_decode_accelerator.cc:333: VideoMemoryTracker::GetInstance(); On 2017/07/14 07:17:32, Pawel Osciak wrote: > Could this be outside of the loop? ditto. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_jpeg_de... File media/gpu/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:370: // TODO(hiroh): The name passed to VideoMemoryTracker On 2017/07/14 07:17:32, Pawel Osciak wrote: > Please file an issue for this and link to it here. Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:376: if (input_buffer_memory_ == nullptr) { On 2017/07/14 07:17:32, Pawel Osciak wrote: > if (!input_buffer_memory_) Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:387: LOG(ERROR) << "getting buffer information failed"; On 2017/07/14 07:17:32, Pawel Osciak wrote: > Do we need to PostNotifyError? Here and below. Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:398: input_buffer_map_[i].address[j] = buffer_info[i][j].address; On 2017/07/14 07:17:32, Pawel Osciak wrote: > Perhaps just store BufferData directly in the map? Please see a comment in V4L2SVDA. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... File media/gpu/v4l2_mmap_memory.cc (right): https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:17: uint32_t count, On 2017/07/14 07:17:32, Pawel Osciak wrote: > count argument should not be needed here. Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:41: video_mem_tracker->RegisterAllocation(allocated_size, name); On 2017/07/14 07:17:32, Pawel Osciak wrote: > Should we verify the return value? Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... File media/gpu/v4l2_mmap_memory.h (right): https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:11: #include "base/callback.h" On 2017/07/14 07:17:33, Pawel Osciak wrote: > Is this header needed? Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:20: // This class must be created only through Create(). On 2017/07/14 07:17:33, Pawel Osciak wrote: > This comment is probably not needed, since the code indicates this by making > constructors private. Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:26: class V4L2MmapMemory { On 2017/07/14 07:17:33, Pawel Osciak wrote: > Is this class thread safe? Do we need a sequence checker? This class writes and reads only member variables, except VideoMemoryTracker one's. VideoMemoryTracker is a thread safe class. The public functions are Create() and GetBufferInfo(). GetBufferInfo just returns buffer_info_, which is completely read operation. That function can be called after Create() is completed. Create() calls several member functions, which are thread safe. Therefore, this class is a thread safe. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:28: // Return unique_ptr of new V4L2MmapMemory. On 2017/07/14 07:17:32, Pawel Osciak wrote: > This sentence is not needed. Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:35: // chrome://tracing (memory-infra). On 2017/07/14 07:17:33, Pawel Osciak wrote: > We should mention that this both allocates and maps the memory. Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:37: // * reqbufs.memory is not V4L2_MEMORY_MMAP. On 2017/07/14 07:17:33, Pawel Osciak wrote: > This is not applicable here anymore. Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:55: // Because buffer_info_ is initialized previously in Allocate(), On 2017/07/14 07:17:33, Pawel Osciak wrote: > This sentence is not needed. Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:62: DISALLOW_IMPLICIT_CONSTRUCTORS(V4L2MmapMemory); On 2017/07/14 07:17:33, Pawel Osciak wrote: > Please place this at the end of the private section. Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:66: uint32_t v4l2_buf_type); On 2017/07/14 07:17:32, Pawel Osciak wrote: > s/uint32_t/enum v4l2_buf_type/ Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:72: // Before calling Allocate, device_, type_ and count_ must be initialized On 2017/07/14 07:17:33, Pawel Osciak wrote: > This sentence is not needed; we can't call Allocate() if the constructor did not > execute. Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:86: const uint32_t count_; On 2017/07/14 07:17:32, Pawel Osciak wrote: > count_ does not need to be stored. Please pass count directly to Allocate() from > Create(). Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:87: const uint32_t v4l2_buf_type_; On 2017/07/14 07:17:33, Pawel Osciak wrote: > enum v4l2_buf_type Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:94: std::unique_ptr<VideoMemoryTrackerHandle> vm_hundle_; On 2017/07/14 07:17:32, Pawel Osciak wrote: > s/hundle/handle/ Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_slice_v... File media/gpu/v4l2_slice_video_decode_accelerator.cc (right): https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_slice_v... media/gpu/v4l2_slice_video_decode_accelerator.cc:762: // should be acquired from VDA::Config. On 2017/07/14 07:17:33, Pawel Osciak wrote: > Please add an issue. Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_slice_v... media/gpu/v4l2_slice_video_decode_accelerator.cc:776: LOG(ERROR) << "getting buffer information failed"; On 2017/07/14 07:17:34, Pawel Osciak wrote: > NOTIFY_ERROR? Here and below. Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_slice_v... media/gpu/v4l2_slice_video_decode_accelerator.cc:781: if (buffer_info[i].size() != 1) { On 2017/07/14 07:17:34, Pawel Osciak wrote: > input_planes_count_? Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_slice_v... media/gpu/v4l2_slice_video_decode_accelerator.cc:782: VLOG(1) << "The number of allocated planes is not 1."; On 2017/07/14 07:17:34, Pawel Osciak wrote: > Is this not a critical error? Probably critical error? Because it means Ioctl didn't work as we expected and memory leak will also happen. So I add NOTIFY_ERROR. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_slice_v... media/gpu/v4l2_slice_video_decode_accelerator.cc:785: input_buffer_map_[i].address = buffer_info[i][0].address; On 2017/07/14 07:17:33, Pawel Osciak wrote: > Could we just store BufferData? The type of input_buffer_map_ is InputRecord. It contains, in addition to address and length, input_id, bytes_used and at_device. Therefore, we can only replace address and length with BufferData. I think there is no advantage to store BufferData in InputRecord, because the other values (e.g. input_id) are remained in InputRecord. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_slice_v... media/gpu/v4l2_slice_video_decode_accelerator.cc:1565: // the output buffers should be deallocated by the usual method. On 2017/07/14 07:17:33, Pawel Osciak wrote: > should be deallocated explicitly here, instead of being released via the > V4L2MmapMemory. Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_slice_v... media/gpu/v4l2_slice_video_decode_accelerator.cc:1566: // To avoid deallocating twice, the if context is necessary here. On 2017/07/14 07:17:33, Pawel Osciak wrote: > This sentence is not needed. Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_slice_v... media/gpu/v4l2_slice_video_decode_accelerator.cc:1612: output_buffer_memory_ = V4L2MmapMemory::Create( On 2017/07/14 07:17:33, Pawel Osciak wrote: > This would result in mapping allocated memory into the userspace via mmap(), but > we don't use that and should not be doing that. These buffers may not always be > mappable and they may be very big, so this could consume considerable amounts of > virtual memory address space. Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_video_d... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:371: output_buffer_memory_ = V4L2MmapMemory::Create( On 2017/07/14 07:17:34, Pawel Osciak wrote: > The previous comment in V4L2SVDA also applies here. Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:2167: return false; On 2017/07/14 07:17:34, Pawel Osciak wrote: > NOTIFY_ERROR? Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:2174: input_buffer_map_[i].address = buffer_info[i][0].address; On 2017/07/14 07:17:34, Pawel Osciak wrote: > Similar comments as in V4L2SVDA. Please see a comment in V4L2SVDA. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_video_e... File media/gpu/v4l2_video_encode_accelerator.cc (right): https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_video_e... media/gpu/v4l2_video_encode_accelerator.cc:1260: // TODO(hiroh): The name passed to VideoMemoryTracker On 2017/07/14 07:17:34, Pawel Osciak wrote: > Please add an issue number. Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/v4l2_video_e... media/gpu/v4l2_video_encode_accelerator.cc:1283: output_buffer_map_[i].address = buffer_info[i][0].address; On 2017/07/14 07:17:34, Pawel Osciak wrote: > Could we store BufferData? Please see a comment in V4L2SVDA. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... File media/gpu/video_memory_tracker.cc (right): https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:7: #include "base/callback.h" On 2017/07/14 07:17:34, Pawel Osciak wrote: > Is this used? Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:36: DCHECK(tracker_task_runner_->BelongsToCurrentThread()); On 2017/07/14 07:17:34, Pawel Osciak wrote: > Could sequencechecker be used for this? What is an advantage to use sequencechecker for this. We have to add sequencechecker to member variables to use it. TaskRunner is already used at other points and checking can be done using this without sequencechecker. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:40: auto size = mem.second; On 2017/07/14 07:17:34, Pawel Osciak wrote: > const auto? Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:57: void VideoMemoryTracker::IncreaseAllocation(const size_t size, On 2017/07/14 07:17:34, Pawel Osciak wrote: > Perhaps in>line this in RegisterAllocation? TrackerTaskRunner should process increasing (allocations_[name] += size). So PostTask is necessary in RegisterMemory. However, it is impossible since the return value of RegisterMemory is not void. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:65: DCHECK(tracker_task_runner_->BelongsToCurrentThread()); On 2017/07/14 07:17:34, Pawel Osciak wrote: > This DCHECK would always be true given the above if() Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:77: DCHECK(tracker_task_runner_->BelongsToCurrentThread()); On 2017/07/14 07:17:34, Pawel Osciak wrote: > ditto Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:83: DCHECK_GE(it->second, size); On 2017/07/14 07:17:34, Pawel Osciak wrote: > This should probably be an if Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... File media/gpu/video_memory_tracker.h (right): https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:11: #include "base/memory/singleton.h" On 2017/07/14 07:17:35, Pawel Osciak wrote: > Do we need to include singleton.h in .h file? Perhaps we could do this only in > .cc. Please see > https://cs.chromium.org/chromium/src/base/memory/singleton.h?rcl=e9e0a9019bcf.... Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:12: #include "base/threading/thread.h" On 2017/07/14 07:17:35, Pawel Osciak wrote: > We should be able to only just declare SingleThreadTaskRunner? Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:20: // VDA and VEA implementations. On 2017/07/14 07:17:35, Pawel Osciak wrote: > This could also be JDA. But I don't think anything about this class is specific > to VDA/VEA/JDA, so maybe it's better to say it gathers and tracks video codec > memory allocations done by its clients, especially since you say below "Any > class can track their own memory by using this class." Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:30: // MemoryDumpProvider implementation On 2017/07/14 07:17:35, Pawel Osciak wrote: > Nit: please dots at the end of sentences. Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:34: // Register memory information that the memory whose name is |name| is On 2017/07/14 07:17:35, Pawel Osciak wrote: > Add |size| bytes to the existing entry of |name| on allocation list, or add a > new one if none such exists. Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:42: void IncreaseAllocation(const size_t size, const std::string& name); On 2017/07/14 07:17:35, Pawel Osciak wrote: > Could we do this in VMT::RegisterAllocation and make this private? Done. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:67: class VideoMemoryTrackerHandle { On 2017/07/14 07:17:34, Pawel Osciak wrote: > Could this be a subclass of VMT? V4L2MmapMemory needs VideoMemoryTrackerHandle. I avoid including video_memory_tracker in v4l2_mma_memory.h by declaring "class VideoMemoryTrackerHandle." It is impossible to declare like "class VideoMemoryTracker::VideoMemoryTrackerHandle." https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:69: VideoMemoryTrackerHandle(const size_t size, const std::string& name) On 2017/07/14 07:17:35, Pawel Osciak wrote: > Perhaps instead pass a closure with size and name bound to it, and just call it > in destructor? DecreaseMemorySize wouldn't have to be public either then. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by hiroh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
As far as I investigated, there is no thread except GPU main thread that tasks in VideoMemoryTracker can be posted. Therefore, I decided to create an own thread in VideoMemoryTracker and post tasks to it. Re: primiano@, can you review VideoMemoryTracker again, since it is changed (it is almost same as the previous version you reviewed though). Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by hiroh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Because VideoMemoryTracker create a new thread, it can be initialized safely at any place. Therefore, we deleted several lines where VideoMemoryTracker::GetInstance() is called. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... File media/gpu/video_memory_tracker.h (right): https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:59: // Tracker task runner. This is a task runner of a GPU main thread. On 2017/07/14 07:17:35, Pawel Osciak wrote: > Do we need this tracker to run on GPUMain? It would be preferable to not block > GPUMain if not needed. There is no appropriate thread except GPU main thread that tasks in VideoMemoryTracker can be posted. Therefore, I decided to create an own thread in VideoMemoryTrackert and post tasks to it.
I am sorry to submit CL twice in a short while. I noticed my mistake and modified them.
The CQ bit was checked by hiroh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hiroh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hiroh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I changed as using V4L2MmapMemory for V4L2SVDA Output Buffer.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_optional_gpu_tests_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_opti...)
Update the document about |mapping| in v4l2_mmap_memory.h
The CQ bit was checked by posciak@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... File media/gpu/video_memory_tracker.cc (right): https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:36: DCHECK(tracker_task_runner_->BelongsToCurrentThread()); On 2017/07/18 04:12:11, hiroh wrote: > On 2017/07/14 07:17:34, Pawel Osciak wrote: > > Could sequencechecker be used for this? > > What is an advantage to use sequencechecker for this. > We have to add sequencechecker to member variables to use it. > > TaskRunner is already used at other points and checking can be done using this > without sequencechecker. Would we in general be able to use a sequence instead of a thread (https://chromium.googlesource.com/chromium/src/+/master/docs/threading_and_ta...) ? https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... File media/gpu/video_memory_tracker.h (right): https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:67: class VideoMemoryTrackerHandle { On 2017/07/18 04:12:12, hiroh wrote: > On 2017/07/14 07:17:34, Pawel Osciak wrote: > > Could this be a subclass of VMT? > > V4L2MmapMemory needs VideoMemoryTrackerHandle. > I avoid including video_memory_tracker in v4l2_mma_memory.h by declaring "class > VideoMemoryTrackerHandle." > It is impossible to declare like "class > VideoMemoryTracker::VideoMemoryTrackerHandle." Yes, please use full name without declaring. I suggest VideoMemoryTracker::Handle. https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_jpeg_de... File media/gpu/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:387: VLOG(1) << "getting buffer information failed"; Please update comments and log messages in the whole CL to be full sentences with proper capitalization and punctuation (per coding style). https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:457: VLOG(1) << "getting buffer information failed"; PostNotifyError()? https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:464: VLOG(1) << "The number of planes != output_buffer_num_planes"; PostNotifyError()? https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:472: .GetArea()) { PostNotifyError()? https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_mmap_me... File media/gpu/v4l2_mmap_memory.cc (right): https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:73: buffer_info_.resize(reqbufs.count); Perhaps just emplace_back as well in the loop below, without resizing here to avoid this if? https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:104: Deallocate(); Should we also early return? If we fail here mid-way, I think we would Deallocate(), but still return allocated_size not equal to 0? We would also potentially Deallocate() multiple times? https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:106: buffer_info_[i].emplace_back(address, buffer.m.planes[j].length); We shouldn't be doing this if we failed to map. https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_mmap_me... File media/gpu/v4l2_mmap_memory.h (right): https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:33: // |mapping| is a flag whether the allocated memory is mapped Perhaps "when true, also map the allocated memory into userspace". https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:38: // that in user space. mmap() only if mapping == true, but the whole sentence could be removed. https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:57: // allocated and mmaped in Allocate(). s/mmapped/mapped/ https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_slice_v... File media/gpu/v4l2_slice_video_decode_accelerator.cc (right): https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_slice_v... media/gpu/v4l2_slice_video_decode_accelerator.cc:783: VLOGF(1) << "The number of allocated planes is not 1."; Please instead print input_planes_count_. https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_video_d... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:2174: NOTIFY_ERROR(PLATFORM_FAILURE); return false? https://codereview.chromium.org/2909533003/diff/640001/media/gpu/video_memory... File media/gpu/video_memory_tracker.cc (right): https://codereview.chromium.org/2909533003/diff/640001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:34: LOG(ERROR) << __func__ << ": tracker thread failed to start"; Would we like to use VLOGS here as well? https://codereview.chromium.org/2909533003/diff/640001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:62: VideoMemoryTracker::RegisterAllocation(size_t size, const std::string name) { const string&? https://codereview.chromium.org/2909533003/diff/640001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:65: return nullptr; I would remove the DCHECK if 0 is handled here. size_t is unsigned. Btw., what should happen if name is an empty string? https://codereview.chromium.org/2909533003/diff/640001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:70: base::Unretained(this), size, name)); Please comment why Unretained is safe. https://codereview.chromium.org/2909533003/diff/640001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:74: const std::string name) { const string&? https://codereview.chromium.org/2909533003/diff/640001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:85: const std::string name) { const string&? https://codereview.chromium.org/2909533003/diff/640001/media/gpu/video_memory... File media/gpu/video_memory_tracker.h (right): https://codereview.chromium.org/2909533003/diff/640001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:25: // The callback should be VMT::DecreaseAllocation. What is passed as the callback should probably not be a concern of this class, since it's an argument for the client to provide. The class has an interface saying "please give me a callback that I should call when I'm destroyed". This sentence could thus be removed. I wonder if we could also just use ScopedGeneric<> for this, instead of creating a new class? https://codereview.chromium.org/2909533003/diff/640001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:28: VideoMemoryTrackerHandle(base::Closure decrease_cb); s/decrease_cb/destruction_cb/ https://codereview.chromium.org/2909533003/diff/640001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:52: // or add ad new one if none such exists. Return VideoMemoryTrackerHandle, s/ad new/a new/ ? https://codereview.chromium.org/2909533003/diff/640001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:56: const std::string name); const&? https://codereview.chromium.org/2909533003/diff/640001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:65: void IncreaseAllocation(const size_t size, const std::string name); const string&? https://codereview.chromium.org/2909533003/diff/640001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:69: void DecreaseAllocation(const size_t size, const std::string name); const string&?
I arranged comments and log messages in whole CL. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... File media/gpu/video_memory_tracker.cc (right): https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:36: DCHECK(tracker_task_runner_->BelongsToCurrentThread()); On 2017/07/28 04:52:58, Pawel Osciak wrote: > On 2017/07/18 04:12:11, hiroh wrote: > > On 2017/07/14 07:17:34, Pawel Osciak wrote: > > > Could sequencechecker be used for this? > > > > What is an advantage to use sequencechecker for this. > > We have to add sequencechecker to member variables to use it. > > > > TaskRunner is already used at other points and checking can be done using this > > without sequencechecker. > > Would we in general be able to use a sequence instead of a thread > (https://chromium.googlesource.com/chromium/src/+/master/docs/threading_and_ta...) > ? We decided, talking with Pawel offline, SequenceTaskRunner was not used here. https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... File media/gpu/video_memory_tracker.h (right): https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:67: class VideoMemoryTrackerHandle { On 2017/07/28 04:52:58, Pawel Osciak wrote: > On 2017/07/18 04:12:12, hiroh wrote: > > On 2017/07/14 07:17:34, Pawel Osciak wrote: > > > Could this be a subclass of VMT? > > > > V4L2MmapMemory needs VideoMemoryTrackerHandle. > > I avoid including video_memory_tracker in v4l2_mma_memory.h by declaring > "class > > VideoMemoryTrackerHandle." > > It is impossible to declare like "class > > VideoMemoryTracker::VideoMemoryTrackerHandle." > > Yes, please use full name without declaring. I suggest > VideoMemoryTracker::Handle. Done. https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_jpeg_de... File media/gpu/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:387: VLOG(1) << "getting buffer information failed"; On 2017/07/28 04:52:58, Pawel Osciak wrote: > Please update comments and log messages in the whole CL to be full sentences > with proper capitalization and punctuation (per coding style). Done. https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:457: VLOG(1) << "getting buffer information failed"; On 2017/07/28 04:52:58, Pawel Osciak wrote: > PostNotifyError()? Done. https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:464: VLOG(1) << "The number of planes != output_buffer_num_planes"; On 2017/07/28 04:52:58, Pawel Osciak wrote: > PostNotifyError()? Done. https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:472: .GetArea()) { On 2017/07/28 04:52:58, Pawel Osciak wrote: > PostNotifyError()? Done. https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_mmap_me... File media/gpu/v4l2_mmap_memory.cc (right): https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:73: buffer_info_.resize(reqbufs.count); On 2017/07/28 04:52:58, Pawel Osciak wrote: > Perhaps just emplace_back as well in the loop below, without resizing here to > avoid this if? Hmm, how do I do that? I also considered I could do so, but I didn't came up with any idea. This is because if statement is executed every j-loop. Should I write like below? for (size_t i = 0; i < reqbufs.count; i++) { // ... for (size_t j = 0; j < buffer.length; j++) { allocated_size += buffer.m.planes[j].length; } if (mapping) { buffer_info_.emplace_back(); for (size_t j = 0; j < buffer.length; j++) { void* address = device_->Mmap(NULL, buffer.m.planes[j].length, PROT_READ | PROT_WRITE, MAP_SHARED, buffer.m.planes[j].m.mem_offset); if (address == MAP_FAILED) { VLOGF(1) << "mmap() failed"; buffer_info_.clear(); Deallocate(); return 0; } buffer_info_[i].emplace_back(address, buffer.m.planes[j].length); } } } https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:104: Deallocate(); On 2017/07/28 04:52:58, Pawel Osciak wrote: > Should we also early return? If we fail here mid-way, I think we would > Deallocate(), but still return allocated_size not equal to 0? We would also > potentially Deallocate() multiple times? Done. https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.cc:106: buffer_info_[i].emplace_back(address, buffer.m.planes[j].length); On 2017/07/28 04:52:58, Pawel Osciak wrote: > We shouldn't be doing this if we failed to map. Done. https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_mmap_me... File media/gpu/v4l2_mmap_memory.h (right): https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:33: // |mapping| is a flag whether the allocated memory is mapped On 2017/07/28 04:52:58, Pawel Osciak wrote: > Perhaps "when true, also map the allocated memory into userspace". Done. https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:38: // that in user space. On 2017/07/28 04:52:58, Pawel Osciak wrote: > mmap() only if mapping == true, but the whole sentence could be removed. Done. https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_mmap_me... media/gpu/v4l2_mmap_memory.h:57: // allocated and mmaped in Allocate(). On 2017/07/28 04:52:58, Pawel Osciak wrote: > s/mmapped/mapped/ Done. https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_slice_v... File media/gpu/v4l2_slice_video_decode_accelerator.cc (right): https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_slice_v... media/gpu/v4l2_slice_video_decode_accelerator.cc:783: VLOGF(1) << "The number of allocated planes is not 1."; On 2017/07/28 04:52:59, Pawel Osciak wrote: > Please instead print input_planes_count_. Done. https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_video_d... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2909533003/diff/640001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:2174: NOTIFY_ERROR(PLATFORM_FAILURE); On 2017/07/28 04:52:59, Pawel Osciak wrote: > return false? Done. https://codereview.chromium.org/2909533003/diff/640001/media/gpu/video_memory... File media/gpu/video_memory_tracker.cc (right): https://codereview.chromium.org/2909533003/diff/640001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:34: LOG(ERROR) << __func__ << ": tracker thread failed to start"; On 2017/07/28 04:52:59, Pawel Osciak wrote: > Would we like to use VLOGS here as well? Done. https://codereview.chromium.org/2909533003/diff/640001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:62: VideoMemoryTracker::RegisterAllocation(size_t size, const std::string name) { On 2017/07/28 04:52:59, Pawel Osciak wrote: > const string&? Done. https://codereview.chromium.org/2909533003/diff/640001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:65: return nullptr; On 2017/07/28 04:52:59, Pawel Osciak wrote: > I would remove the DCHECK if 0 is handled here. size_t is unsigned. > > Btw., what should happen if name is an empty string? If name is an empty, it is just inserted into Allocations. It causes no problem, but empty name is strange. Then, should I insert DCHECK(!name.empty())? https://codereview.chromium.org/2909533003/diff/640001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:70: base::Unretained(this), size, name)); On 2017/07/28 04:52:59, Pawel Osciak wrote: > Please comment why Unretained is safe. Done. https://codereview.chromium.org/2909533003/diff/640001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:74: const std::string name) { On 2017/07/28 04:52:59, Pawel Osciak wrote: > const string&? Done. https://codereview.chromium.org/2909533003/diff/640001/media/gpu/video_memory... media/gpu/video_memory_tracker.cc:85: const std::string name) { On 2017/07/28 04:52:59, Pawel Osciak wrote: > const string&? I am concerned the lifetime of |name|. This function is always called through Callback. According to the document, even if an argument is a reference, it is copied until it is passed using ConstRef. Therefore, I think I should select the safer solution, because this copy cost is not too high. https://chromium.googlesource.com/chromium/src/+/lkcr/docs/callback.md#Passin... https://codereview.chromium.org/2909533003/diff/640001/media/gpu/video_memory... File media/gpu/video_memory_tracker.h (right): https://codereview.chromium.org/2909533003/diff/640001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:25: // The callback should be VMT::DecreaseAllocation. On 2017/07/28 04:52:59, Pawel Osciak wrote: > What is passed as the callback should probably not be a concern of this class, > since it's an argument for the client to provide. The class has an interface > saying "please give me a callback that I should call when I'm destroyed". This > sentence could thus be removed. > > I wonder if we could also just use ScopedGeneric<> for this, instead of creating > a new class? https://cs.chromium.org/chromium/src/base/scoped_generic.h ScopedGeneric<> is a expansion of unique_ptr whose custom deleter must be given. Deleter in ScopedGeneric<> has to be a class that some functions need to be provided. So we cannot use this in order not to create a new class. On the other hand, a custom deleter for unique_ptr should be only a function object. Callback is not a function object. It should be run via Run(). So that DecreaseAllocation() is a function object, it must be a static function. However, it cannot be static, because it needs to operate on |allocations_|. As far as I search, there is no class like a custom deleter for callback function in Chromium. Hmm, hence I think it is unavoidable to create this class until RegitserAllocation returns callback, which primiano@ disagreed such a desgin. https://codereview.chromium.org/2909533003/diff/640001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:28: VideoMemoryTrackerHandle(base::Closure decrease_cb); On 2017/07/28 04:52:59, Pawel Osciak wrote: > s/decrease_cb/destruction_cb/ Done. https://codereview.chromium.org/2909533003/diff/640001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:52: // or add ad new one if none such exists. Return VideoMemoryTrackerHandle, On 2017/07/28 04:52:59, Pawel Osciak wrote: > s/ad new/a new/ ? Done. https://codereview.chromium.org/2909533003/diff/640001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:56: const std::string name); On 2017/07/28 04:52:59, Pawel Osciak wrote: > const&? Done. https://codereview.chromium.org/2909533003/diff/640001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:65: void IncreaseAllocation(const size_t size, const std::string name); On 2017/07/28 04:52:59, Pawel Osciak wrote: > const string&? Done. https://codereview.chromium.org/2909533003/diff/640001/media/gpu/video_memory... media/gpu/video_memory_tracker.h:69: void DecreaseAllocation(const size_t size, const std::string name); On 2017/07/28 04:52:59, Pawel Osciak wrote: > const string&? See a comment in video_memory_tracker.cc
The CQ bit was checked by hiroh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run. |