Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1645873002:
Use ParamTraits for media::BitstreamBuffer. (Closed)
|
Created:
4 years, 10 months ago by Owen Lin Modified:
4 years, 9 months ago CC:
danakj+watch_chromium.org, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, jbauman+watch_chromium.org, kalyank, mcasas+watch_chromium.org, piman+watch_chromium.org, posciak+watch_chromium.org, sievers+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse ParamTraits for media::BitstreamBuffer.
BUG=None
TEST=Play YouTube on a Chrome OS device
Committed: https://crrev.com/25eced9ee2b1580bb6700ce833ef19261bd83e21
Cr-Commit-Position: refs/heads/master@{#378422}
Patch Set 1 : Ready for review #
Total comments: 8
Patch Set 2 : Fix compiling errors on mac and address review comments #
Total comments: 11
Patch Set 3 : address review comments #Patch Set 4 : rebase over crrev.com/1706023003 #Patch Set 5 : rebase #Messages
Total messages: 41 (19 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Using friends. Use ParamTraits for media::BitstreamBuffer. BUG= ========== to ========== Use ParamTraits for media::BitstreamBuffer. BUG=None ==========
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Use ParamTraits for media::BitstreamBuffer. BUG=None ========== to ========== Use ParamTraits for media::BitstreamBuffer. BUG=None TEST=Play YouTube on Chrome OS device ==========
owenlin@chromium.org changed reviewers: + ccameron@chromium.org, dcheng@chromium.org, kcwu@chromium.org, posciak@chromium.org, xhwang@chromium.org
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Use ParamTraits for media::BitstreamBuffer. BUG=None TEST=Play YouTube on Chrome OS device ========== to ========== Use ParamTraits for media::BitstreamBuffer. BUG=None TEST=Play YouTube on a Chrome OS device ==========
PTAL. Thanks. This CL is to address the review comments (mainly from dcheng) in https://codereview.chromium.org/1541353002/.
The CQ bit was checked by owenlin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1645873002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1645873002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/1645873002/diff/60001/content/common/gpu/gpu_... File content/common/gpu/gpu_messages.h (right): https://codereview.chromium.org/1645873002/diff/60001/content/common/gpu/gpu_... content/common/gpu/gpu_messages.h:156: IPC_STRUCT_MEMBER(media::BitstreamBuffer, bitstream_buffer) How about named it as input_buffer or input_bitstream_buffer? Make its role clearer. https://codereview.chromium.org/1645873002/diff/60001/content/common/gpu/medi... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1645873002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:202: if (base::SharedMemory::IsHandleValid(params.bitstream_buffer.handle())) No longer need to validate params.bitstream_buffer.handle() ? However, this looks inconsistent to params.output_video_frame_handle which need to validate. Maybe worth one line comment to explain.
c/c/g/m lgtm % nits https://codereview.chromium.org/1645873002/diff/60001/content/common/gpu/clie... File content/common/gpu/client/gpu_video_decode_accelerator_host.cc (right): https://codereview.chromium.org/1645873002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_video_decode_accelerator_host.cc:122: media::BitstreamBuffer bsb = bitstream_buffer; Perhaps s/bsb/buffer_to_send/ https://codereview.chromium.org/1645873002/diff/60001/content/common/gpu/gpu_... File content/common/gpu/gpu_messages.h (right): https://codereview.chromium.org/1645873002/diff/60001/content/common/gpu/gpu_... content/common/gpu/gpu_messages.h:58: #ifndef CONTENT_COMMON_GPU_GPU_MESSAGE_H_ s/MESSAGE/MESSAGES/ ?
media/ lgtm % other's comments.
media/ lgtm % other's comments.
Patchset #2 (id:80001) has been deleted
dcheng@chromium.org changed reviewers: + erikchen@chromium.org
https://codereview.chromium.org/1645873002/diff/100001/content/common/gpu/cli... File content/common/gpu/client/gpu_video_decode_accelerator_host.cc (right): https://codereview.chromium.org/1645873002/diff/100001/content/common/gpu/cli... content/common/gpu/client/gpu_video_decode_accelerator_host.cc:123: base::SharedMemoryHandle handle = I think the IPC subsystem has been improved so you don't need to explicitly dupe anymore, but erikchen@ should confirm. https://codereview.chromium.org/1645873002/diff/100001/content/common/gpu/gpu... File content/common/gpu/gpu_messages.cc (right): https://codereview.chromium.org/1645873002/diff/100001/content/common/gpu/gpu... content/common/gpu/gpu_messages.cc:35: if (!base::IsValueInRangeForNumericType<size_t>(size)) { #include <stddef.h> Also, I think it would be slightly more canonical to write this as: CheckedNumeric<size_t> checked_size(size); if (!checked_size.IsValid()) return false; r->size_ = checked_size.ValueOrDie(); (which means including safe_math.h from //base/numerics) https://codereview.chromium.org/1645873002/diff/100001/content/common/gpu/gpu... content/common/gpu/gpu_messages.cc:36: LOG(ERROR) << "Invalid size: " << size; #include "base/logging.h" Also, let's make this (and other logging statements in this file) DLOG(). https://codereview.chromium.org/1645873002/diff/100001/content/common/gpu/gpu... content/common/gpu/gpu_messages.cc:59: std::ostringstream oss; #include <sstream> https://codereview.chromium.org/1645873002/diff/100001/content/common/gpu/gpu... File content/common/gpu/gpu_messages.h (right): https://codereview.chromium.org/1645873002/diff/100001/content/common/gpu/gpu... content/common/gpu/gpu_messages.h:58: #ifndef CONTENT_COMMON_GPU_GPU_MESSAGES_H_ Often times, people just add this to a new file (something like gpu_param_traits.h). But either way is probably fine.
https://codereview.chromium.org/1645873002/diff/100001/content/common/gpu/cli... File content/common/gpu/client/gpu_video_decode_accelerator_host.cc (right): https://codereview.chromium.org/1645873002/diff/100001/content/common/gpu/cli... content/common/gpu/client/gpu_video_decode_accelerator_host.cc:123: base::SharedMemoryHandle handle = On 2016/02/18 21:06:09, dcheng wrote: > I think the IPC subsystem has been improved so you don't need to explicitly dupe > anymore, but erikchen@ should confirm. correct. no need to dupe shared memory handles before passing them to ipc.
We are also working on another related CL. https://codereview.chromium.org/1706023003/ which moves the validation of bitstream_buffer's handle and id into each VDA implementation. Since it is part of the VDA interface. After that, the ParamTraits will be responsible to verify both "offset" and "size" are in valid range. Each VDA is responsible to verify the id and handle. https://codereview.chromium.org/1645873002/diff/60001/content/common/gpu/clie... File content/common/gpu/client/gpu_video_decode_accelerator_host.cc (right): https://codereview.chromium.org/1645873002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_video_decode_accelerator_host.cc:122: media::BitstreamBuffer bsb = bitstream_buffer; On 2016/02/16 08:48:02, Pawel Osciak wrote: > Perhaps s/bsb/buffer_to_send/ Done. https://codereview.chromium.org/1645873002/diff/60001/content/common/gpu/gpu_... File content/common/gpu/gpu_messages.h (right): https://codereview.chromium.org/1645873002/diff/60001/content/common/gpu/gpu_... content/common/gpu/gpu_messages.h:58: #ifndef CONTENT_COMMON_GPU_GPU_MESSAGE_H_ On 2016/02/16 08:48:02, Pawel Osciak wrote: > s/MESSAGE/MESSAGES/ ? Done. https://codereview.chromium.org/1645873002/diff/60001/content/common/gpu/gpu_... content/common/gpu/gpu_messages.h:156: IPC_STRUCT_MEMBER(media::BitstreamBuffer, bitstream_buffer) On 2016/02/16 08:44:18, kcwu wrote: > How about named it as input_buffer or input_bitstream_buffer? Make its role > clearer. Done. https://codereview.chromium.org/1645873002/diff/60001/content/common/gpu/medi... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1645873002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:202: if (base::SharedMemory::IsHandleValid(params.bitstream_buffer.handle())) On 2016/02/16 08:44:18, kcwu wrote: > No longer need to validate params.bitstream_buffer.handle() ? > > However, this looks inconsistent to params.output_video_frame_handle which need > to validate. Maybe worth one line comment to explain. We are going to move the check into VDAs and JDAs. So, let's still keep the checks here. https://codereview.chromium.org/1645873002/diff/100001/content/common/gpu/cli... File content/common/gpu/client/gpu_video_decode_accelerator_host.cc (right): https://codereview.chromium.org/1645873002/diff/100001/content/common/gpu/cli... content/common/gpu/client/gpu_video_decode_accelerator_host.cc:123: base::SharedMemoryHandle handle = On 2016/02/18 21:07:31, erikchen wrote: > On 2016/02/18 21:06:09, dcheng wrote: > > I think the IPC subsystem has been improved so you don't need to explicitly > dupe > > anymore, but erikchen@ should confirm. > > correct. no need to dupe shared memory handles before passing them to ipc. Thanks. That's great. But I think we should do that in anther CL where I am going to remove the function GpuChannelHost::ShareToGpuProcess(), since it is not needed. https://codereview.chromium.org/1645873002/diff/100001/content/common/gpu/gpu... File content/common/gpu/gpu_messages.cc (right): https://codereview.chromium.org/1645873002/diff/100001/content/common/gpu/gpu... content/common/gpu/gpu_messages.cc:35: if (!base::IsValueInRangeForNumericType<size_t>(size)) { On 2016/02/18 21:06:09, dcheng wrote: > #include <stddef.h> > > Also, I think it would be slightly more canonical to write this as: > CheckedNumeric<size_t> checked_size(size); > if (!checked_size.IsValid()) return false; > r->size_ = checked_size.ValueOrDie(); > > (which means including safe_math.h from //base/numerics) Done. https://codereview.chromium.org/1645873002/diff/100001/content/common/gpu/gpu... content/common/gpu/gpu_messages.cc:36: LOG(ERROR) << "Invalid size: " << size; On 2016/02/18 21:06:09, dcheng wrote: > #include "base/logging.h" > > Also, let's make this (and other logging statements in this file) DLOG(). Done. https://codereview.chromium.org/1645873002/diff/100001/content/common/gpu/gpu... content/common/gpu/gpu_messages.cc:59: std::ostringstream oss; On 2016/02/18 21:06:09, dcheng wrote: > #include <sstream> Done. https://codereview.chromium.org/1645873002/diff/100001/content/common/gpu/gpu... File content/common/gpu/gpu_messages.h (right): https://codereview.chromium.org/1645873002/diff/100001/content/common/gpu/gpu... content/common/gpu/gpu_messages.h:58: #ifndef CONTENT_COMMON_GPU_GPU_MESSAGES_H_ On 2016/02/18 21:06:09, dcheng wrote: > Often times, people just add this to a new file (something like > gpu_param_traits.h). But either way is probably fine. Acknowledged.
ipc lgtm
owenlin@chromium.org changed reviewers: + jbauman@chromium.org
PTAL. Thanks.
ipc still lgtm but... the patch you rebased over is going the wrong way, imo. the idea of ParamTraits is to centralize more checks so we don't have to repeat them in all the VDA implementations.
On 2016/02/24 18:47:58, dcheng wrote: > ipc still lgtm but... > > the patch you rebased over is going the wrong way, imo. the idea of ParamTraits > is to centralize more checks so we don't have to repeat them in all the VDA > implementations. There are several reasons for the decision. First and most important: The contract (interface) between the VDA and VDA::Client about Decode() is: if calling VDA::Decode() with an invalid BitstreamBuffer, the VDA should call Client::NotifyError(INVALID_ARGUMENT) to let client know the error. Since this is the interface, all the implementations should do it. Besides, I believe when designing VDA, the VDA is not aware of the existence of this IPC path. And the existing IPC won't be the only path to call VDAs, we are going to add mojo IPC for ARC in the near future. Second, it is better to do the validation where the argument is used. So that it is more easy to understand why is the validation and there won't be multiple validations along the calling path. Last, it is not trivial to deal with the errors in IPC. When ParaTraits::Read returns false, it calls IPC::Listener::onBadMessageReceived. So we need to see if the bad message is for Decode() and calls Client::NotifyError(). The things because more complicated when a filter is involved. Besides, there is no way if the contract is to call NotifyError(INVALID_ARGUMENT) for invalid ID and to call(UNREADABLE_INPUT) for an invalid handle. I am sorry if you feel like we did the modification after your lgtm, But I did described the details related in comment #21. I did both CLs in parallel and the other (1706023003) was just landed yesterday.
On 2016/02/25 at 02:44:46, owenlin wrote: > On 2016/02/24 18:47:58, dcheng wrote: > > ipc still lgtm but... > > > > the patch you rebased over is going the wrong way, imo. the idea of ParamTraits > > is to centralize more checks so we don't have to repeat them in all the VDA > > implementations. > > There are several reasons for the decision. > > First and most important: The contract (interface) between the VDA and VDA::Client about Decode() is: if calling VDA::Decode() with an invalid BitstreamBuffer, the VDA should call Client::NotifyError(INVALID_ARGUMENT) to let client know the error. Since this is the interface, all the implementations should do it. Besides, I believe when designing VDA, the VDA is not aware of the existence of this IPC path. And the existing IPC won't be the only path to call VDAs, we are going to add mojo IPC for ARC in the near future. Sure, but the same comments that apply to IPC also apply to Mojo: we should try to minimize the amount of duplicated validation (because that's just more chances that one code path will forget to validate). > > Second, it is better to do the validation where the argument is used. So that it is more easy to understand why is the validation and there won't be multiple validations along the calling path. > There shouldn't be multiple validations along the path since it should all happen in the same place: ideally ParamTraits. > Last, it is not trivial to deal with the errors in IPC. When ParaTraits::Read returns false, it calls IPC::Listener::onBadMessageReceived. So we need to see if the bad message is for Decode() and calls Client::NotifyError(). The things because more complicated when a filter is involved. Besides, there is no way if the contract is to call NotifyError(INVALID_ARGUMENT) for invalid ID and to call(UNREADABLE_INPUT) for an invalid handle. > This makes sense. We'll have to make sure the story is better for Mojo. That being said... why is it OK for the renderer to send an invalid bitstream buffer ID to the GPU process? Is there some sort of race that can legitimately happen here? Or can this only happen as a result of a renderer purposely sending bad values? > I am sorry if you feel like we did the modification after your lgtm, But I did described the details related in comment #21. I did both CLs in parallel and the other (1706023003) was just landed yesterday.
On 2016/02/25 21:27:18, dcheng wrote: > On 2016/02/25 at 02:44:46, owenlin wrote: > > On 2016/02/24 18:47:58, dcheng wrote: > > > ipc still lgtm but... > > > > > > the patch you rebased over is going the wrong way, imo. the idea of > ParamTraits > > > is to centralize more checks so we don't have to repeat them in all the VDA > > > implementations. > > > > There are several reasons for the decision. > > > > First and most important: The contract (interface) between the VDA and > VDA::Client about Decode() is: if calling VDA::Decode() with an invalid > BitstreamBuffer, the VDA should call Client::NotifyError(INVALID_ARGUMENT) to > let client know the error. Since this is the interface, all the implementations > should do it. Besides, I believe when designing VDA, the VDA is not aware of the > existence of this IPC path. And the existing IPC won't be the only path to call > VDAs, we are going to add mojo IPC for ARC in the near future. > > Sure, but the same comments that apply to IPC also apply to Mojo: we should try > to minimize the amount of duplicated validation (because that's just more > chances that one code path will forget to validate). If duplicate codes is really a concern. I think another choice is to have an abstract class for VDA who do the check. In that case, no matter how many different VDAs and how many differents IPC subsystem. There will just one place to do the check. > > > > > Second, it is better to do the validation where the argument is used. So that > it is more easy to understand why is the validation and there won't be multiple > validations along the calling path. > > > > There shouldn't be multiple validations along the path since it should all > happen in the same place: ideally ParamTraits. > > > Last, it is not trivial to deal with the errors in IPC. When ParaTraits::Read > returns false, it calls IPC::Listener::onBadMessageReceived. So we need to see > if the bad message is for Decode() and calls Client::NotifyError(). The things > because more complicated when a filter is involved. Besides, there is no way if > the contract is to call NotifyError(INVALID_ARGUMENT) for invalid ID and to > call(UNREADABLE_INPUT) for an invalid handle. > > > > This makes sense. We'll have to make sure the story is better for Mojo. That > being said... why is it OK for the renderer to send an invalid bitstream buffer > ID to the GPU process? Is there some sort of race that can legitimately happen > here? Or can this only happen as a result of a renderer purposely sending bad > values? That's really the point, how should VDA handle an invalid stream? I think Pawel should knows better about it. I think it is good for a public API to return errors if there is problem in the argument. But it is hard to say VDA is a public API. > > > I am sorry if you feel like we did the modification after your lgtm, But I did > described the details related in comment #21. I did both CLs in parallel and the > other (1706023003) was just landed yesterday.
content/common/gpu lgtm stamp
The CQ bit was checked by owenlin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org, posciak@chromium.org Link to the patchset: https://codereview.chromium.org/1645873002/#ps140001 (title: "rebase over crrev.com/1706023003")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1645873002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1645873002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by owenlin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ccameron@chromium.org, dcheng@chromium.org, xhwang@chromium.org, posciak@chromium.org Link to the patchset: https://codereview.chromium.org/1645873002/#ps160001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1645873002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1645873002/160001
Message was sent while issue was closed.
Description was changed from ========== Use ParamTraits for media::BitstreamBuffer. BUG=None TEST=Play YouTube on a Chrome OS device ========== to ========== Use ParamTraits for media::BitstreamBuffer. BUG=None TEST=Play YouTube on a Chrome OS device ==========
Message was sent while issue was closed.
Committed patchset #5 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Use ParamTraits for media::BitstreamBuffer. BUG=None TEST=Play YouTube on a Chrome OS device ========== to ========== Use ParamTraits for media::BitstreamBuffer. BUG=None TEST=Play YouTube on a Chrome OS device Committed: https://crrev.com/25eced9ee2b1580bb6700ce833ef19261bd83e21 Cr-Commit-Position: refs/heads/master@{#378422} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/25eced9ee2b1580bb6700ce833ef19261bd83e21 Cr-Commit-Position: refs/heads/master@{#378422} |
