Refactor media/video shared memory usage |
|
Issue descriptionThe base::SharedMemory/base::SharedMemoryHandle api is being deprecated, see blocked bug for details. The rough strategy is: * Add the new shared memory regions to VideoFrame. * Update BitstreamBuffer to use the new API. * Update MojoVideoEncoder (see crbug.com/793446). All subject to change, of course, depending on what I find as I refactor.
,
Jun 8 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6072f0f6545dede7e60fbd5fe291179be4d376dd commit 6072f0f6545dede7e60fbd5fe291179be4d376dd Author: Matthew Cary <mattcary@chromium.org> Date: Fri Jun 08 07:55:07 2018 media/base: Add new shared memory api support to VideoFrame. This extends VideoFrame to be able to use the new shared memory API. It does not remove support for the legacy API, so that clients can be migrated individually. Bug: 849207 Change-Id: Ibd80588c57bf65143dd249edd8d2b4ef620c9132 Reviewed-on: https://chromium-review.googlesource.com/1084485 Reviewed-by: Dan Sanders <sandersd@chromium.org> Commit-Queue: Matthew Cary <mattcary@chromium.org> Cr-Commit-Position: refs/heads/master@{#565581} [modify] https://crrev.com/6072f0f6545dede7e60fbd5fe291179be4d376dd/media/base/video_frame.cc [modify] https://crrev.com/6072f0f6545dede7e60fbd5fe291179be4d376dd/media/base/video_frame.h [modify] https://crrev.com/6072f0f6545dede7e60fbd5fe291179be4d376dd/media/base/video_frame_unittest.cc
,
Jun 27 2018
After some amount of refactoring and testing I think I have hit upon a good plan of action. Two key types involved with this are the BitstreamBuffer and UnalignedSharedMemory. The latter is wraps the mapping of the shared memory region transported by the BitstreamBuffer, and owns the shared memory region. The BitstreamBuffer is the class which transports shared memory regions, and unfortunately has deeply integrated a lot of the problems with the legacy shared memory API which we are fixing with the new API. First, the ownership semantics of the buffer are not clear, and are not reflected in the type system (eg, const BitstreamBuffers rely on the const-ness of SharedMemoryHandle::Close to mutate instances). Second, the protection level of the shared memory region (read-only/writable) is not specified in the type, and some BitstreamBuffers are set to read-only in ways that cause the same problems that crbug.com/795291 describes. Changing this all at once would involve an unrealistically large CL that touches code across all platforms. As the proposed changes fixed subtle things like shared memory handle ownership, it seems imperative to structure this with many small CLs that can be independently reverted if something goes awry. The first stage of this work will be to change BitstreamBuffer to transport an UnsafeSharedMemoryRegion rather than the handle it currently transports. It does not appear possible for BitstreamBuffer to use WritableSharedMemoryRegion or ReadOnlySharedMemoryRegion (as would be preferred). Using a ReadOnlySharedMemoryRegion is eliminated because encoders must write data into the buffer. A WritableSharedMemoryRegion seems plausible, but will require significant work on some platforms to keep a list of writable mappings. Hence we will convert BitstreamBuffer to hold an UnsafeSharedMemoryRegion. Because of the ownership semantic changes mentioned above, it is not practical to make the BitstreamBuffer change first. Instead, all clients will be updated individually. Then a multi-platform CL which is essentially just a name change will be done. The first step is to create a new flavor of UnalignedSharedMemory, UnsafeUnalignedSharedMemory. Where the old UnalignedSharedMemory type is created from a legacy SharedMemoryHandle, this new type is created from a new shared memory API instance of UnsafeSharedMemoryRegion. This changes both the ownership semantics and mapping details from UnalignedSharedMemory: the new UnsafeUnalignedSharedMemory does not own the region backing the shared memory, and only owns the mapping itself. Because of this, the region is mapped upon construction rather than with a later MapAt call. These changes do not seem to affect behavior in a practical manner. This change also removes the read-only bit. This is no longer meaningful, as only the mapping is owned; in addition the original use of this variable was not correct, as explained in detail in https://goo.gl/HmBYy6. (TL;DR: on some platforms, being read-only is a characteristic of the underlying OS region, and cannot be set in some processes without affecting the protection in all processes. As this behavior is incompatible with usage, this means that sometimes shared memory which has been declared read-only is not, in fact, read-only, and could be exploited by rouge processes). Finally, a the new UnsafeUnalignedSharedMemory contains a shim constructor which takes a legacy SharedMemoryHandle. The ownership semantics are the same a the region-created version, that is, the handle is *not* owned by the UnsafeUnalignedSharedMemory instance. This shim allows us to convert all users of this class one-by-one, and confirm that the new ownership semantics are correctly handled. After this is done, the BitstreamBuffer class, which is at the heart of the IPCs involved, can be converted to the new API without needing to change ownership semantics at the same time.
,
Jun 27 2018
> The first step is to create a new flavor of UnalignedSharedMemory, UnsafeUnalignedSharedMemory. > > Where the old UnalignedSharedMemory type is created from a legacy SharedMemoryHandle, this new type is created from a new shared memory API instance of UnsafeSharedMemoryRegion. This changes both the ownership semantics and mapping details from UnalignedSharedMemory: the new UnsafeUnalignedSharedMemory does not own the region backing the shared memory, and only owns the mapping itself. Because of this, the region is mapped upon construction rather than with a later MapAt call. I think it'd be better to call it UnalignedWritableSharedMemoryMapping then, since it would be a just a wrapper around WritableSharedMemoryMapping.
,
Jun 27 2018
Not a bad point. Let me add you to the CL and we can discuss there.
,
Jun 27 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4254e2b22c1d770eb410a02955ddca4d7182bfa7 commit 4254e2b22c1d770eb410a02955ddca4d7182bfa7 Author: Matthew Cary <mattcary@chromium.org> Date: Wed Jun 27 23:46:37 2018 media/base: Add new UnalignedWritableMapping type. Where the old UnalignedSharedMemory type is created from a legacy SharedMemoryHandle, this new type is created from a new shared memory API instance of UnsafeSharedMemoryRegion. This changes both the ownership semantics and mapping details from UnalignedSharedMemory: the new UnalignedWritableMapping does not own the region backing the shared memory, and only owns the mapping itself. Because of this, the region is mapped upon construction rather than with a later MapAt call. These changes do not seem to affect behavior in a practical manner. This change also removes the read-only bit. This is no longer meaningful, as only the mapping is owned; in addition the original use of this variable was not correct, as explained in detail in https://goo.gl/HmBYy6. (TL;DR: on some platforms, being read-only is a characteristic of the underlying OS region, and cannot be set in some processes without affecting the protection in all processes. As this behavior is incompatible with usage, this means that sometimes shared memory which has been declared read-only is not, in fact, read-only, and could be exploited by rouge processes). Finally, a the new UnalignedWritableMapping contains a shim constructor which takes a legacy SharedMemoryHandle. The ownership semantics are the same a the region-created version, that is, the handle is *not* owned by the UnalignedWritableMapping instance. This shim allows us to convert all users of this class one-by-one, and confirm that the new ownership semantics are correctly handled. After this is done, the BitstreamBuffer class, which is at the heart of the IPCs involved, can be converted to the new API without needing to change ownership semantics at the same time. See the bug for an overview of the entire process. Bug: 849207 Change-Id: Ibe4bbb48f7bea3fb31728bf98133ede0228b9801 Reviewed-on: https://chromium-review.googlesource.com/1116703 Reviewed-by: Ken Rockot <rockot@chromium.org> Reviewed-by: Dan Sanders <sandersd@chromium.org> Reviewed-by: Alexandr Ilin <alexilin@chromium.org> Commit-Queue: Matthew Cary <mattcary@chromium.org> Cr-Commit-Position: refs/heads/master@{#570930} [modify] https://crrev.com/4254e2b22c1d770eb410a02955ddca4d7182bfa7/media/DEPS [modify] https://crrev.com/4254e2b22c1d770eb410a02955ddca4d7182bfa7/media/base/unaligned_shared_memory.cc [modify] https://crrev.com/4254e2b22c1d770eb410a02955ddca4d7182bfa7/media/base/unaligned_shared_memory.h [modify] https://crrev.com/4254e2b22c1d770eb410a02955ddca4d7182bfa7/media/base/unaligned_shared_memory_unittest.cc
,
Jun 29 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2a8045471d8d0434dba7b1f57e5f3d4c0623ac4e commit 2a8045471d8d0434dba7b1f57e5f3d4c0623ac4e Author: Matthew Cary <mattcary@chromium.org> Date: Fri Jun 29 08:40:44 2018 media/gpu shared memory: Update android/ UnalignedSharedMemory. Uses the new WritableUnalignedMapping class, which changes the ownership semantics. See the bug for details on the overall plan. Bug: 849207 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Change-Id: Ie8da3246a5c7036322e72e120c4925cebc451f0e Reviewed-on: https://chromium-review.googlesource.com/1117685 Reviewed-by: Frank Liberato <liberato@chromium.org> Commit-Queue: Matthew Cary <mattcary@chromium.org> Cr-Commit-Position: refs/heads/master@{#571427} [modify] https://crrev.com/2a8045471d8d0434dba7b1f57e5f3d4c0623ac4e/media/gpu/android/android_video_decode_accelerator.cc [modify] https://crrev.com/2a8045471d8d0434dba7b1f57e5f3d4c0623ac4e/media/gpu/android/android_video_decode_accelerator.h [modify] https://crrev.com/2a8045471d8d0434dba7b1f57e5f3d4c0623ac4e/media/gpu/android/android_video_encode_accelerator.cc
,
Jul 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6538a9011f717450bbc96eec5d1bb0ad8e788dc3 commit 6538a9011f717450bbc96eec5d1bb0ad8e788dc3 Author: Matthew Cary <mattcary@chromium.org> Date: Mon Jul 02 09:07:01 2018 media/base WritableUnalignedMapping fix. Use the underlying handle size when wrapping a legacy shared memory instance. Bug: 849207 Change-Id: Ibbf62ebbea5a95d64d84a0faf2042f639be5b820 Reviewed-on: https://chromium-review.googlesource.com/1118268 Reviewed-by: Alexandr Ilin <alexilin@chromium.org> Reviewed-by: Dan Sanders <sandersd@chromium.org> Commit-Queue: Matthew Cary <mattcary@chromium.org> Cr-Commit-Position: refs/heads/master@{#571846} [modify] https://crrev.com/6538a9011f717450bbc96eec5d1bb0ad8e788dc3/media/base/unaligned_shared_memory.cc [modify] https://crrev.com/6538a9011f717450bbc96eec5d1bb0ad8e788dc3/media/base/unaligned_shared_memory_unittest.cc
,
Jul 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0c6afb9046aeca975f1ed023dd91841cacd36278 commit 0c6afb9046aeca975f1ed023dd91841cacd36278 Author: Matthew Cary <mattcary@chromium.org> Date: Mon Jul 02 12:33:36 2018 media/gpu shared memory: Update fake UnalignedSharedMemory. Uses the new WritableUnalignedMapping class, which changes the ownership semantics. See the bug for details on the overall plan. Bug: 849207 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Change-Id: I84bf28d0da5b6c6d9e764b6f30d48f263550e37f Reviewed-on: https://chromium-review.googlesource.com/1117689 Reviewed-by: Dan Sanders <sandersd@chromium.org> Commit-Queue: Matthew Cary <mattcary@chromium.org> Cr-Commit-Position: refs/heads/master@{#571872} [modify] https://crrev.com/0c6afb9046aeca975f1ed023dd91841cacd36278/media/gpu/fake_jpeg_decode_accelerator.cc [modify] https://crrev.com/0c6afb9046aeca975f1ed023dd91841cacd36278/media/gpu/fake_jpeg_decode_accelerator.h
,
Jul 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1cf21ca052c1cb68179c4df4e767cb45239ae467 commit 1cf21ca052c1cb68179c4df4e767cb45239ae467 Author: Matthew Cary <mattcary@chromium.org> Date: Wed Jul 18 17:36:15 2018 media/gpu: Add mojo/edk initialization to unittest. This adds mojo/edk initialization to media/gpu/video_decode_accelerator_unittest. This is necessary for future changes that use mojo machinary for shared memory manipulation. Bug: 849207 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Change-Id: I6d04891a4d306f7c458eb2bb4ce3fd42a1b77759 Reviewed-on: https://chromium-review.googlesource.com/1122095 Commit-Queue: Matthew Cary <mattcary@chromium.org> Reviewed-by: Pawel Osciak <posciak@chromium.org> Reviewed-by: Ken Rockot <rockot@chromium.org> Cr-Commit-Position: refs/heads/master@{#576121} [modify] https://crrev.com/1cf21ca052c1cb68179c4df4e767cb45239ae467/media/gpu/BUILD.gn [modify] https://crrev.com/1cf21ca052c1cb68179c4df4e767cb45239ae467/media/gpu/DEPS [modify] https://crrev.com/1cf21ca052c1cb68179c4df4e767cb45239ae467/media/gpu/video_decode_accelerator_unittest.cc
,
Jul 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a73dae7e65cbd017db50fddfc9acf458b7d7a313 commit a73dae7e65cbd017db50fddfc9acf458b7d7a313 Author: Matthew Cary <mattcary@chromium.org> Date: Wed Jul 25 08:35:18 2018 media/gpu shared memory: update vaapi UnalignedSharedMemory Uses the new WritableUnalignedMapping class, which changes the ownership semantics. See the bug for details on the overall plan. Bug: 849207 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Change-Id: Ie3379f1dabb0c0fa7fae7451534bd5fa1b01416e Reviewed-on: https://chromium-review.googlesource.com/1117696 Reviewed-by: Daniele Castagna <dcastagna@chromium.org> Reviewed-by: Alexandr Ilin <alexilin@chromium.org> Commit-Queue: Matthew Cary <mattcary@chromium.org> Cr-Commit-Position: refs/heads/master@{#577823} [modify] https://crrev.com/a73dae7e65cbd017db50fddfc9acf458b7d7a313/media/gpu/vaapi/vaapi_jpeg_decode_accelerator.cc [modify] https://crrev.com/a73dae7e65cbd017db50fddfc9acf458b7d7a313/media/gpu/vaapi/vaapi_jpeg_encode_accelerator.cc [modify] https://crrev.com/a73dae7e65cbd017db50fddfc9acf458b7d7a313/media/gpu/vaapi/vaapi_jpeg_encode_accelerator.h [modify] https://crrev.com/a73dae7e65cbd017db50fddfc9acf458b7d7a313/media/gpu/vaapi/vaapi_video_decode_accelerator.cc [modify] https://crrev.com/a73dae7e65cbd017db50fddfc9acf458b7d7a313/media/gpu/vaapi/vaapi_video_encode_accelerator.cc
,
Jul 27
#11 - after #11, apollolake chromebook cannot use hardware video decoding. [4456:4456:0727/140314.772439:ERROR:shared_memory_handle_posix.cc(43)] close:: Bad file descriptor (9) The close failure happens in the call stack. #2 0x5c6f7fffbf8d base::SharedMemoryHandle::Close() #3 0x5c6f7e1c49ae media::VaapiVideoDecodeAccelerator::QueueInputBuffer() #4 0x5c6f7e1c6357 media::VaapiVideoDecodeAccelerator::Decode() #5 0x5c6f8140cb7e _ZN3IPC8MessageTI38AcceleratedVideoDecoderMsg_Decode_MetaNSt3__15tupleIJN5media15BitstreamBufferEEEEvE8DispatchINS4_25GpuVideoDecodeAcceleratorES9_vMS9_FvRKS5_EEEbPKNS_7MessageEPT_PT0_PT1_T2_ #6 0x5c6f8140ca31 media::GpuVideoDecodeAccelerator::OnMessageReceived() #7 0x5c6f838ceae7 gpu::GpuChannel::HandleMessageHelper() #8 0x5c6f838cd1c9 gpu::GpuChannel::HandleMessage() #9 0x5c6f838c1963 gpu::Scheduler::RunNextTask() #10 0x5c6f7fffe739 base::debug::TaskAnnotator::RunTask() #11 0x5c6f7ff71252 base::MessageLoop::RunTask() #12 0x5c6f7ff71727 base::MessageLoop::DoWork() #13 0x5c6f7ff7276a base::MessagePumpDefault::Run() #14 0x5c6f7ff90935 base::RunLoop::Run() #15 0x5c6f831f446f content::GpuMain() #16 0x5c6f7fc39ff6 content::ContentMainRunnerImpl::Run() #17 0x5c6f7fc4115c service_manager::Main() #18 0x5c6f7fc38151 content::ContentMain() #19 0x5c6f7d9aaeff ChromeMain #20 0x7fe111434736 __libc_start_main #21 0x5c6f7d9aad29 _start
,
Jul 30
But I don't think this is where the trap happens? That error should just be informational, while the reverted CL did double-close handles it didn't extend the lifetime of anything.
,
Jul 31
The problem is because the unittest doesn't init mojo (mojo::core::Init()). I think this is going to be a general problem; the new shared memory region stuff implicitly depends on mojo, and while we could work around the immediate problem by implementing a non-mojo SharedHandle->WritableMapping converter, I suspect that there will be other problems with the shared memory system assuming mojo is available.
,
Aug 1
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2f61858f9b1e43ee5643703b172cabbfc7dd9bf8 commit 2f61858f9b1e43ee5643703b172cabbfc7dd9bf8 Author: Matthew Cary <mattcary@chromium.org> Date: Wed Aug 01 10:55:27 2018 media/base: Fix incorrect comment in WritableUnalignedMapping. When WritableUnalignedMapping takes a handle, it *does* own it (the temporary shared memory region created in the constructor consumes the handle). Bug: 849207 Change-Id: Icabca455727ad275d9c5a55065c5baf18f44e08c Reviewed-on: https://chromium-review.googlesource.com/1152913 Reviewed-by: Dan Sanders <sandersd@chromium.org> Reviewed-by: Alexandr Ilin <alexilin@chromium.org> Commit-Queue: Matthew Cary <mattcary@chromium.org> Cr-Commit-Position: refs/heads/master@{#579755} [modify] https://crrev.com/2f61858f9b1e43ee5643703b172cabbfc7dd9bf8/media/base/unaligned_shared_memory.h
,
Aug 6
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e4378cb927e3b673ac49dd9c0272b88b58027d2d commit e4378cb927e3b673ac49dd9c0272b88b58027d2d Author: Matthew Cary <mattcary@chromium.org> Date: Mon Aug 06 18:16:39 2018 media/gpu: add mojo initialization to unittests. Future changes to shared memory introduce an implicit dependency on mojo, which must be initialized in unittests. Bug: 849207 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Change-Id: I13de882b7f995ac33385d2f6d5f49b5ac44bb2c4 Reviewed-on: https://chromium-review.googlesource.com/1156588 Reviewed-by: Miguel Casas <mcasas@chromium.org> Reviewed-by: Dan Sanders <sandersd@chromium.org> Commit-Queue: Matthew Cary <mattcary@chromium.org> Cr-Commit-Position: refs/heads/master@{#580922} [modify] https://crrev.com/e4378cb927e3b673ac49dd9c0272b88b58027d2d/media/gpu/BUILD.gn [modify] https://crrev.com/e4378cb927e3b673ac49dd9c0272b88b58027d2d/media/gpu/jpeg_decode_accelerator_unittest.cc [modify] https://crrev.com/e4378cb927e3b673ac49dd9c0272b88b58027d2d/media/gpu/jpeg_encode_accelerator_unittest.cc [modify] https://crrev.com/e4378cb927e3b673ac49dd9c0272b88b58027d2d/media/gpu/vaapi/BUILD.gn [modify] https://crrev.com/e4378cb927e3b673ac49dd9c0272b88b58027d2d/media/gpu/vaapi/vaapi_jpeg_decoder_unittest.cc [modify] https://crrev.com/e4378cb927e3b673ac49dd9c0272b88b58027d2d/media/gpu/video_decode_accelerator_unittest.cc [modify] https://crrev.com/e4378cb927e3b673ac49dd9c0272b88b58027d2d/media/gpu/video_encode_accelerator_unittest.cc
,
Aug 6
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5ccc9e5db90f1b6fabfe29b42f80a9f397650267 commit 5ccc9e5db90f1b6fabfe29b42f80a9f397650267 Author: Ahmed Fakhry <afakhry@chromium.org> Date: Mon Aug 06 20:32:36 2018 Revert "media/gpu: add mojo initialization to unittests." This reverts commit e4378cb927e3b673ac49dd9c0272b88b58027d2d. Reason for revert: breaks BuildPackages on amd64-generic-goma-canary-chromium-pfq-informational BUG: chromium:871429 Original change's description: > media/gpu: add mojo initialization to unittests. > > Future changes to shared memory introduce an implicit dependency on > mojo, which must be initialized in unittests. > > Bug: 849207 > Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel > Change-Id: I13de882b7f995ac33385d2f6d5f49b5ac44bb2c4 > Reviewed-on: https://chromium-review.googlesource.com/1156588 > Reviewed-by: Miguel Casas <mcasas@chromium.org> > Reviewed-by: Dan Sanders <sandersd@chromium.org> > Commit-Queue: Matthew Cary <mattcary@chromium.org> > Cr-Commit-Position: refs/heads/master@{#580922} TBR=posciak@chromium.org,mcasas@chromium.org,sandersd@chromium.org,mattcary@chromium.org Change-Id: Iaa263cfca7e99b36c609db6b4a81dbd56d1a9be3 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 849207 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Reviewed-on: https://chromium-review.googlesource.com/1164182 Reviewed-by: Ahmed Fakhry <afakhry@chromium.org> Commit-Queue: Ahmed Fakhry <afakhry@chromium.org> Cr-Commit-Position: refs/heads/master@{#580973} [modify] https://crrev.com/5ccc9e5db90f1b6fabfe29b42f80a9f397650267/media/gpu/BUILD.gn [modify] https://crrev.com/5ccc9e5db90f1b6fabfe29b42f80a9f397650267/media/gpu/jpeg_decode_accelerator_unittest.cc [modify] https://crrev.com/5ccc9e5db90f1b6fabfe29b42f80a9f397650267/media/gpu/jpeg_encode_accelerator_unittest.cc [modify] https://crrev.com/5ccc9e5db90f1b6fabfe29b42f80a9f397650267/media/gpu/vaapi/BUILD.gn [modify] https://crrev.com/5ccc9e5db90f1b6fabfe29b42f80a9f397650267/media/gpu/vaapi/vaapi_jpeg_decoder_unittest.cc [modify] https://crrev.com/5ccc9e5db90f1b6fabfe29b42f80a9f397650267/media/gpu/video_decode_accelerator_unittest.cc [modify] https://crrev.com/5ccc9e5db90f1b6fabfe29b42f80a9f397650267/media/gpu/video_encode_accelerator_unittest.cc
,
Aug 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7dfa0458bcc86c0e8a5d5c769fd480473a737b2e commit 7dfa0458bcc86c0e8a5d5c769fd480473a737b2e Author: Matthew Cary <mattcary@chromium.org> Date: Wed Aug 08 03:43:21 2018 media/gpu: add mojo initialization to unittests. Future changes to shared memory introduce an implicit dependency on mojo, which must be initialized in unittests. Reland of crrev.com/c/1156588 with proper includes. TBR: mcasas@chromium.org Bug: 849207 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Change-Id: I06220d263deab02c82f97ca72a52cf9066b3df29 Reviewed-on: https://chromium-review.googlesource.com/1165984 Commit-Queue: Matthew Cary <mattcary@chromium.org> Reviewed-by: Dan Sanders <sandersd@chromium.org> Cr-Commit-Position: refs/heads/master@{#581464} [modify] https://crrev.com/7dfa0458bcc86c0e8a5d5c769fd480473a737b2e/media/gpu/BUILD.gn [modify] https://crrev.com/7dfa0458bcc86c0e8a5d5c769fd480473a737b2e/media/gpu/jpeg_decode_accelerator_unittest.cc [modify] https://crrev.com/7dfa0458bcc86c0e8a5d5c769fd480473a737b2e/media/gpu/jpeg_encode_accelerator_unittest.cc [modify] https://crrev.com/7dfa0458bcc86c0e8a5d5c769fd480473a737b2e/media/gpu/vaapi/BUILD.gn [modify] https://crrev.com/7dfa0458bcc86c0e8a5d5c769fd480473a737b2e/media/gpu/vaapi/vaapi_jpeg_decoder_unittest.cc [modify] https://crrev.com/7dfa0458bcc86c0e8a5d5c769fd480473a737b2e/media/gpu/video_decode_accelerator_unittest.cc [modify] https://crrev.com/7dfa0458bcc86c0e8a5d5c769fd480473a737b2e/media/gpu/video_encode_accelerator_unittest.cc
,
Aug 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e30ad9f8f6d1d613173083907501c25232b6c1c3 commit e30ad9f8f6d1d613173083907501c25232b6c1c3 Author: Matthew Cary <mattcary@chromium.org> Date: Fri Aug 10 15:32:43 2018 media/gpu shared memory: Use WritableUnalignedMapping for vaapi. Uses the new WritableUnalignedMapping class, which changes the ownership semantics. See the bug for details on the overall plan. This is a reland of crrev.com/c/1117696, now that mojo is initialized for the unittests is fixed, and also removes a double-close for the shared memory handles. Bug: 849207 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Change-Id: Ib065caedfed8a33fd0f112ce22a72bda4b9bf99e Reviewed-on: https://chromium-review.googlesource.com/1158567 Reviewed-by: Daniele Castagna <dcastagna@chromium.org> Commit-Queue: Matthew Cary <mattcary@chromium.org> Cr-Commit-Position: refs/heads/master@{#582177} [modify] https://crrev.com/e30ad9f8f6d1d613173083907501c25232b6c1c3/media/gpu/vaapi/vaapi_jpeg_decode_accelerator.cc [modify] https://crrev.com/e30ad9f8f6d1d613173083907501c25232b6c1c3/media/gpu/vaapi/vaapi_jpeg_decode_accelerator.h [modify] https://crrev.com/e30ad9f8f6d1d613173083907501c25232b6c1c3/media/gpu/vaapi/vaapi_jpeg_encode_accelerator.cc [modify] https://crrev.com/e30ad9f8f6d1d613173083907501c25232b6c1c3/media/gpu/vaapi/vaapi_jpeg_encode_accelerator.h [modify] https://crrev.com/e30ad9f8f6d1d613173083907501c25232b6c1c3/media/gpu/vaapi/vaapi_video_decode_accelerator.cc [modify] https://crrev.com/e30ad9f8f6d1d613173083907501c25232b6c1c3/media/gpu/vaapi/vaapi_video_encode_accelerator.cc
,
Aug 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cfe635cec3e0b4e3a490a6785711f6b0963ced3a commit cfe635cec3e0b4e3a490a6785711f6b0963ced3a Author: Matthew Cary <mattcary@chromium.org> Date: Fri Aug 10 18:49:10 2018 media/gpu shared memory: update v4l2 UnalignedSharedMemory Uses the new WritableUnalignedMapping class, which changes the ownership semantics. See the bug for details on the overall plan. Bug: 849207 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Change-Id: Idacc45e249291bd9d42ff7bf9a8abffe8636f6cf Reviewed-on: https://chromium-review.googlesource.com/1117692 Commit-Queue: Matthew Cary <mattcary@chromium.org> Reviewed-by: Daniele Castagna <dcastagna@chromium.org> Reviewed-by: Alexandr Ilin <alexilin@chromium.org> Cr-Commit-Position: refs/heads/master@{#582270} [modify] https://crrev.com/cfe635cec3e0b4e3a490a6785711f6b0963ced3a/media/gpu/v4l2/v4l2_jpeg_decode_accelerator.cc [modify] https://crrev.com/cfe635cec3e0b4e3a490a6785711f6b0963ced3a/media/gpu/v4l2/v4l2_jpeg_decode_accelerator.h [modify] https://crrev.com/cfe635cec3e0b4e3a490a6785711f6b0963ced3a/media/gpu/v4l2/v4l2_slice_video_decode_accelerator.cc [modify] https://crrev.com/cfe635cec3e0b4e3a490a6785711f6b0963ced3a/media/gpu/v4l2/v4l2_video_decode_accelerator.cc [modify] https://crrev.com/cfe635cec3e0b4e3a490a6785711f6b0963ced3a/media/gpu/v4l2/v4l2_video_encode_accelerator.cc
,
Aug 16
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d46d015237bd647bc2fde584a376a646beef9a8e commit d46d015237bd647bc2fde584a376a646beef9a8e Author: Hirokazu Honda <hiroh@chromium.org> Date: Thu Aug 16 01:39:21 2018 Revert "media/gpu shared memory: update v4l2 UnalignedSharedMemory" This reverts commit cfe635cec3e0b4e3a490a6785711f6b0963ced3a. Reason for revert: This breaks ARC++ (App and CTS/GTS). See detail in crbug.com/874074 Original change's description: > media/gpu shared memory: update v4l2 UnalignedSharedMemory > > Uses the new WritableUnalignedMapping class, which changes the ownership > semantics. See the bug for details on the overall plan. > > Bug: 849207 > Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel > Change-Id: Idacc45e249291bd9d42ff7bf9a8abffe8636f6cf > Reviewed-on: https://chromium-review.googlesource.com/1117692 > Commit-Queue: Matthew Cary <mattcary@chromium.org> > Reviewed-by: Daniele Castagna <dcastagna@chromium.org> > Reviewed-by: Alexandr Ilin <alexilin@chromium.org> > Cr-Commit-Position: refs/heads/master@{#582270} TBR=dcastagna@chromium.org,mattcary@chromium.org,alexilin@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 849207 Change-Id: I1fdf459f8b480da412854d0a86ece9a0ac86a1c8 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Reviewed-on: https://chromium-review.googlesource.com/1176841 Reviewed-by: Hirokazu Honda <hiroh@chromium.org> Commit-Queue: Hirokazu Honda <hiroh@chromium.org> Cr-Commit-Position: refs/heads/master@{#583487} [modify] https://crrev.com/d46d015237bd647bc2fde584a376a646beef9a8e/media/gpu/v4l2/v4l2_jpeg_decode_accelerator.cc [modify] https://crrev.com/d46d015237bd647bc2fde584a376a646beef9a8e/media/gpu/v4l2/v4l2_jpeg_decode_accelerator.h [modify] https://crrev.com/d46d015237bd647bc2fde584a376a646beef9a8e/media/gpu/v4l2/v4l2_slice_video_decode_accelerator.cc [modify] https://crrev.com/d46d015237bd647bc2fde584a376a646beef9a8e/media/gpu/v4l2/v4l2_video_decode_accelerator.cc [modify] https://crrev.com/d46d015237bd647bc2fde584a376a646beef9a8e/media/gpu/v4l2/v4l2_video_encode_accelerator.cc
,
Jan 8
See crbug.com/838365 where several of these changes were reverted due to crashes on android. After some investigation, I think that the crashes were caused by a race introduced by closing the handle when creating the decoder BitstreamRecord. If the BitstreamBuffer, or its associated handle, is reused, then there will be a use-after-free that would produce this stack trace. I don't quite see how a BitstreamBuffer could be used twice, but also I don't see how a memory region could only occasionally be readonly. A race seems more likely I think. So I think the solution is to clean up the ownership of BitstreamBuffer users, which I am working on now. |
|
►
Sign in to add a comment |
|
Comment 1 by mattcary@chromium.org
, Jun 4 2018