Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(83)

Issue 2909533003: Support Memory-infra for HW video codec allocations on CrOS

Created:
3 years, 7 months ago by hiroh
Modified:
3 years, 4 months ago
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+676 lines, -268 lines) Patch
M media/gpu/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 2 chunks +4 lines, -0 lines 0 comments Download
M media/gpu/v4l2_jpeg_decode_accelerator.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 2 chunks +8 lines, -0 lines 0 comments Download
M media/gpu/v4l2_jpeg_decode_accelerator.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 5 chunks +64 lines, -90 lines 0 comments Download
A media/gpu/v4l2_mmap_memory.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 1 chunk +98 lines, -0 lines 0 comments Download
A media/gpu/v4l2_mmap_memory.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 1 chunk +131 lines, -0 lines 0 comments Download
M media/gpu/v4l2_slice_video_decode_accelerator.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 3 chunks +10 lines, -2 lines 0 comments Download
M media/gpu/v4l2_slice_video_decode_accelerator.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 7 chunks +81 lines, -72 lines 0 comments Download
M media/gpu/v4l2_video_decode_accelerator.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 2 chunks +8 lines, -0 lines 0 comments Download
M media/gpu/v4l2_video_decode_accelerator.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 5 chunks +40 lines, -63 lines 0 comments Download
M media/gpu/v4l2_video_encode_accelerator.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +4 lines, -0 lines 0 comments Download
M media/gpu/v4l2_video_encode_accelerator.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 3 chunks +31 lines, -41 lines 0 comments Download
A media/gpu/video_memory_tracker.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 1 chunk +84 lines, -0 lines 0 comments Download
A media/gpu/video_memory_tracker.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 1 chunk +113 lines, -0 lines 0 comments Download

Messages

Total messages: 100 (45 generated)
hiroh
Please see my document (https://docs.google.com/a/google.com/document/d/1XWN2camYBx7pbrXe-eZpvNQLS357m5qAwJ4HghCffQg/edit?usp=sharing). Because it is WIP, there are probably some mistakes. I ...
3 years, 7 months ago (2017-05-26 07:22:31 UTC) #4
reveman
https://codereview.chromium.org/2909533003/diff/1/media/gpu/v4l2_video_decode_accelerator.cc File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2909533003/diff/1/media/gpu/v4l2_video_decode_accelerator.cc#newcode75 media/gpu/v4l2_video_decode_accelerator.cc:75: this, "V4L2MemoryDumpProvider", nullptr); "V4L2MemoryDumpProvider": The name should be unique ...
3 years, 7 months ago (2017-05-26 15:19:19 UTC) #6
hiroh
I change the design as V4L2Device has a responsibility of V4L2VDA Input/Output Buffers. There is ...
3 years, 6 months ago (2017-05-29 09:26:57 UTC) #7
reveman
I'm really confused by this design. Looks like you're creating a memory dump provider per ...
3 years, 6 months ago (2017-05-31 15:01:00 UTC) #8
hiroh
New design proposal is uploaded. Because this is design proposal, I didn't compile yet. I'm ...
3 years, 6 months ago (2017-06-02 05:50:05 UTC) #10
Pawel Osciak
https://codereview.chromium.org/2909533003/diff/40001/media/gpu/v4l2_mmap_manager.h File media/gpu/v4l2_mmap_manager.h (right): https://codereview.chromium.org/2909533003/diff/40001/media/gpu/v4l2_mmap_manager.h#newcode1 media/gpu/v4l2_mmap_manager.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. ...
3 years, 6 months ago (2017-06-02 08:05:11 UTC) #11
reveman
I like the idea of having some form of VideoMemoryManager but do we need multiple ...
3 years, 6 months ago (2017-06-02 14:40:07 UTC) #12
hiroh
I improved the design referring the comments and also implemented the functions. No test is ...
3 years, 6 months ago (2017-06-03 19:45:10 UTC) #13
hiroh
I add locks in several functions in VideoMemoryManager.
3 years, 6 months ago (2017-06-05 01:03:04 UTC) #14
Pawel Osciak
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#newcode144 media/gpu/BUILD.gn:144: "v4l2_mmap_memory.cc", The v4l2_* files should only be ...
3 years, 6 months ago (2017-06-05 07:37:01 UTC) #15
hiroh
VideoMemoryManager manages the size and name of the memory. Each memory is represented by a ...
3 years, 6 months ago (2017-06-06 05:55:10 UTC) #16
hiroh
Add some comments. https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory_manager.cc File media/gpu/video_memory_manager.cc (right): https://codereview.chromium.org/2909533003/diff/100001/media/gpu/video_memory_manager.cc#newcode67 media/gpu/video_memory_manager.cc:67: memory_name_[index] = name; The present implementation ...
3 years, 6 months ago (2017-06-06 06:01:16 UTC) #17
hiroh
add comments https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_video_decode_accelerator.cc File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_video_decode_accelerator.cc#newcode2507 media/gpu/v4l2_video_decode_accelerator.cc:2507: */ Should deallocation be executed clearly here ...
3 years, 6 months ago (2017-06-06 06:09:29 UTC) #18
Pawel Osciak
https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_memory.h File media/gpu/v4l2_mmap_memory.h (right): https://codereview.chromium.org/2909533003/diff/80001/media/gpu/v4l2_mmap_memory.h#newcode23 media/gpu/v4l2_mmap_memory.h:23: v4l2_requestbuffers* const reqbufs, On 2017/06/06 05:55:09, hiroh wrote: > ...
3 years, 6 months ago (2017-06-07 10:08:14 UTC) #19
hiroh
https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_memory.cc File media/gpu/v4l2_mmap_memory.cc (right): https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_memory.cc#newcode26 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 ...
3 years, 6 months ago (2017-06-08 23:17:43 UTC) #20
Pawel Osciak
https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_memory.cc File media/gpu/v4l2_mmap_memory.cc (right): https://codereview.chromium.org/2909533003/diff/100001/media/gpu/v4l2_mmap_memory.cc#newcode26 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: ...
3 years, 6 months ago (2017-06-09 00:04:30 UTC) #21
hiroh
https://codereview.chromium.org/2909533003/diff/120001/media/gpu/v4l2_mmap_memory.h File media/gpu/v4l2_mmap_memory.h (right): https://codereview.chromium.org/2909533003/diff/120001/media/gpu/v4l2_mmap_memory.h#newcode34 media/gpu/v4l2_mmap_memory.h:34: // less buffers than request is allocated, this returns ...
3 years, 6 months ago (2017-06-09 04:00:14 UTC) #22
hiroh
Besides V4L2 VDA, MMAP memory are dumped also in V4L2 SVDA and V4L2 VEA. I ...
3 years, 6 months ago (2017-06-12 09:33:59 UTC) #23
hiroh
Besides V4L2 VDA, MMAP memory are dumped also in V4L2 SVDA and V4L2 VEA. I ...
3 years, 6 months ago (2017-06-12 09:34:03 UTC) #24
hiroh
I upload vaapi_surface_memory.{cc, h}, which trucks vaSurface memory allocated in VA VDA/VEA. I only define ...
3 years, 6 months ago (2017-06-14 01:38:41 UTC) #25
hiroh
I remove vaapi_surface_memory.{h,cc} so that CL can be separated.
3 years, 6 months ago (2017-06-14 04:55:48 UTC) #26
hiroh
I will upload another patch to track vaSurface. Because the patch includes a lot of ...
3 years, 6 months ago (2017-06-15 01:55:47 UTC) #27
hiroh
It turns out the name of memory will be not unique in the future. The ...
3 years, 6 months ago (2017-06-16 00:38:10 UTC) #28
Pawel Osciak
https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_memory.cc File media/gpu/v4l2_mmap_memory.cc (right): https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_memory.cc#newcode23 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_memory.cc#newcode25 media/gpu/v4l2_mmap_memory.cc:25: const v4l2_requestbuffers& reqbufs, ...
3 years, 6 months ago (2017-06-19 01:51:13 UTC) #29
hiroh
I changed the name VideoMemoryManager to VideoMemoryTracker. https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_memory.cc File media/gpu/v4l2_mmap_memory.cc (right): https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_memory.cc#newcode23 media/gpu/v4l2_mmap_memory.cc:23: std::unique_ptr<V4L2MmapMemory> V4L2MmapMemory::Create( ...
3 years, 6 months ago (2017-06-19 07:06:28 UTC) #30
Pawel Osciak
https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_memory.h File media/gpu/v4l2_mmap_memory.h (right): https://codereview.chromium.org/2909533003/diff/280001/media/gpu/v4l2_mmap_memory.h#newcode26 media/gpu/v4l2_mmap_memory.h:26: class V4L2MmapMemory { On 2017/06/19 07:06:28, hiroh wrote: > ...
3 years, 6 months ago (2017-06-19 09:44:33 UTC) #31
hiroh
VideoMemoryTracker cannot track any memory in the below situation. 1) VDA(VEA) and memory-trace turn on. ...
3 years, 6 months ago (2017-06-20 02:25:11 UTC) #32
reveman
https://codereview.chromium.org/2909533003/diff/300001/media/gpu/v4l2_mmap_memory.h File media/gpu/v4l2_mmap_memory.h (right): https://codereview.chromium.org/2909533003/diff/300001/media/gpu/v4l2_mmap_memory.h#newcode69 media/gpu/v4l2_mmap_memory.h:69: size_t Allocate(); why can't the ctor allocate memory and ...
3 years, 6 months ago (2017-06-20 02:41:52 UTC) #33
hiroh
I fixed, so that VideoMemoryTracker can dump even in the abovementioned situation.
3 years, 6 months ago (2017-06-20 07:14:37 UTC) #34
hiroh
I added sadurl@ to owner to ask my change to gpu/ipc/service/{DEPS, gpu_init.cc} is OK. VideoMemoryTracker ...
3 years, 6 months ago (2017-06-20 08:08:27 UTC) #36
hiroh
My goal is to track all important memory used in video codec allocated for Video ...
3 years, 6 months ago (2017-06-21 00:01:55 UTC) #37
reveman
https://codereview.chromium.org/2909533003/diff/340001/media/gpu/video_memory_tracker.cc File media/gpu/video_memory_tracker.cc (right): https://codereview.chromium.org/2909533003/diff/340001/media/gpu/video_memory_tracker.cc#newcode27 media/gpu/video_memory_tracker.cc:27: base::trace_event::MemoryDumpManager::GetInstance()->UnregisterDumpProvider( I think for sanity the same code that ...
3 years, 6 months ago (2017-06-21 15:10:49 UTC) #39
sadrul
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#newcode2 gpu/ipc/service/DEPS:2: "+media/gpu", I see code in media/gpu depend on code ...
3 years, 6 months ago (2017-06-21 21:15:34 UTC) #40
Primiano Tucci (use gerrit)
Some comments inline below. By quickly glancing at the code I generally agree with the ...
3 years, 6 months ago (2017-06-22 07:39:39 UTC) #41
hiroh
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#newcode2 gpu/ipc/service/DEPS:2: "+media/gpu", On ...
3 years, 6 months ago (2017-06-22 08:21:17 UTC) #42
hiroh
Minor update to fix a future bug and DCHECK().
3 years, 5 months ago (2017-06-27 03:30:17 UTC) #43
hiroh
Rebase. V4L2MmapMemory is used in V4L2 JDA (I previously forgot that). Task runner mechanism is ...
3 years, 5 months ago (2017-07-05 07:30:28 UTC) #44
hiroh
Minor bug is fixed around VideoMemoryTracker::DecreaseAllocation
3 years, 5 months ago (2017-07-06 04:21:20 UTC) #45
sadrul
Removing myself from reviewer list since changes in gpu/ipc/service have gone away.
3 years, 5 months ago (2017-07-06 07:13:18 UTC) #47
Primiano Tucci (use gerrit)
The overall design makes more sense (specifically making the VMT a singleton). However, as-is, this ...
3 years, 5 months ago (2017-07-07 08:51:17 UTC) #48
hiroh
Modify VideoMemoryTracker's design, referring primiano@'s review. https://codereview.chromium.org/2909533003/diff/440001/media/gpu/video_memory_tracker.cc File media/gpu/video_memory_tracker.cc (right): https://codereview.chromium.org/2909533003/diff/440001/media/gpu/video_memory_tracker.cc#newcode38 media/gpu/video_memory_tracker.cc:38: tracker_task_runner_->PostTask( On 2017/07/07 ...
3 years, 5 months ago (2017-07-11 06:08:19 UTC) #49
Primiano Tucci (use gerrit)
The tracker looks good now. My only concern is that starting a brand new thread ...
3 years, 5 months ago (2017-07-11 09:42:59 UTC) #50
hiroh
I set VideoMemoryTracker's task runner is one of GPU main thread. https://codereview.chromium.org/2909533003/diff/460001/media/gpu/video_memory_tracker.cc File media/gpu/video_memory_tracker.cc (right): ...
3 years, 5 months ago (2017-07-12 04:06:12 UTC) #51
hiroh
rebase
3 years, 5 months ago (2017-07-12 05:54:22 UTC) #52
Primiano Tucci (use gerrit)
/media/gpu/video_memory_tracker.* LGTM with some final comments. That seems the only file that interacts with memory-infra. ...
3 years, 5 months ago (2017-07-12 15:04:03 UTC) #53
hiroh
Update! https://codereview.chromium.org/2909533003/diff/500001/media/gpu/video_memory_tracker.cc File media/gpu/video_memory_tracker.cc (right): https://codereview.chromium.org/2909533003/diff/500001/media/gpu/video_memory_tracker.cc#newcode43 media/gpu/video_memory_tracker.cc:43: base::StringPrintf("Video Memory/%s", name.c_str())); On 2017/07/12 15:04:03, Primiano Tucci ...
3 years, 5 months ago (2017-07-13 01:31:50 UTC) #54
Pawel Osciak
https://codereview.chromium.org/2909533003/diff/520001/media/gpu/gpu_video_decode_accelerator_factory.cc File media/gpu/gpu_video_decode_accelerator_factory.cc (right): https://codereview.chromium.org/2909533003/diff/520001/media/gpu/gpu_video_decode_accelerator_factory.cc#newcode8 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_encode_accelerator_factory.cc File media/gpu/gpu_video_encode_accelerator_factory.cc ...
3 years, 5 months ago (2017-07-14 07:17:35 UTC) #62
hiroh
I have one unresolved comment. On 2017/07/14 07:17:35, Pawel Osciak wrote: > Do we need ...
3 years, 5 months ago (2017-07-18 04:12:12 UTC) #65
hiroh
As far as I investigated, there is no thread except GPU main thread that tasks ...
3 years, 5 months ago (2017-07-18 05:45:11 UTC) #70
hiroh
Because VideoMemoryTracker create a new thread, it can be initialized safely at any place. Therefore, ...
3 years, 5 months ago (2017-07-19 00:38:49 UTC) #75
hiroh
I am sorry to submit CL twice in a short while. I noticed my mistake ...
3 years, 5 months ago (2017-07-19 00:42:15 UTC) #76
hiroh
I changed as using V4L2MmapMemory for V4L2SVDA Output Buffer.
3 years, 5 months ago (2017-07-25 06:02:07 UTC) #87
hiroh
Update the document about |mapping| in v4l2_mmap_memory.h
3 years, 5 months ago (2017-07-25 06:18:43 UTC) #90
Pawel Osciak
https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory_tracker.cc File media/gpu/video_memory_tracker.cc (right): https://codereview.chromium.org/2909533003/diff/520001/media/gpu/video_memory_tracker.cc#newcode36 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 ...
3 years, 4 months ago (2017-07-28 04:52:59 UTC) #95
hiroh
3 years, 4 months ago (2017-07-31 07:24:36 UTC) #96
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

Powered by Google App Engine
This is Rietveld 408576698