Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2513973002:
Use mojo typemap to simplify the code using DmabufPlane (Closed)
|
Created:
4 years, 1 month ago by yoshiki Modified:
4 years ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, posciak+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, piman+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse mojo typemap to simplify the code using DmabufPlane
This patch introduces a typemap for video_accelerator.mojom struct and allows to use arc::ArcVideoAcceleratorDmabufPlane directly without having to convert from the arc::mojom::DmabufPlane.
BUG=665723
TEST=Ran YT and confirmed that histograms/Media.ArcGpuVideoDecodeAccelerator.InitializeResult is zero on chell and minnie
Committed: https://crrev.com/2f600a079c2fb7e8d9b37e6c246100d5ecdd59f7
Cr-Commit-Position: refs/heads/master@{#438512}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Addressed comments #
Total comments: 23
Patch Set 3 : Addressed comments #
Total comments: 6
Patch Set 4 : Rebased #Patch Set 5 : Addressed comments #Messages
Total messages: 78 (50 generated)
The CQ bit was checked by yoshiki@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.
Description was changed from ========== . BUG= ========== to ========== Use mojo typemaps to simplify GpuArcVideoService::BindDmabuf() BUG=665723' ==========
yoshiki@chromium.org changed reviewers: + yusukes%chromium.org@gtempaccount.com
Yusuke-san, could you take a first look?
Yusuke-san, ping
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org, yusukes@chromium.org - yusukes%chromium.org@gtempaccount.com
adding the real yusukes@.
On 2016/11/29 18:10:24, Luis Héctor Chávez wrote: > adding the real yusukes@. Ah, thanks.
drive-by https://codereview.chromium.org/2513973002/diff/1/chrome/gpu/gpu_arc_video_se... File chrome/gpu/gpu_arc_video_service.cc (right): https://codereview.chromium.org/2513973002/diff/1/chrome/gpu/gpu_arc_video_se... chrome/gpu/gpu_arc_video_service.cc:248: std::vector<::arc::ArcVideoAcceleratorDmabufPlane> planes(1); You can maybe directly initialize the plane with an initializer list: std::vector<::arc::ArcVideoAcceleratorDmabufPlane> planes{ {0, stride} }; https://codereview.chromium.org/2513973002/diff/1/chrome/gpu/gpu_arc_video_se... chrome/gpu/gpu_arc_video_service.cc:266: // TODO(yusukes): Use mojo typemaps to simplify the code. you can now safely remove this, and the whole L267-L274 block if you also do the comment in the typemap code. https://codereview.chromium.org/2513973002/diff/1/components/arc/video_accele... File components/arc/video_accelerator/video_accelerator.h (right): https://codereview.chromium.org/2513973002/diff/1/components/arc/video_accele... components/arc/video_accelerator/video_accelerator.h:14: } nit: empty line before to preserve symmetry. https://codereview.chromium.org/2513973002/diff/1/components/arc/video_accele... File components/arc/video_accelerator/video_accelerator_struct_traits.h (right): https://codereview.chromium.org/2513973002/diff/1/components/arc/video_accele... components/arc/video_accelerator/video_accelerator_struct_traits.h:26: out->stride = data.stride(); here you can check if either the stride or offset are invalid and log + return false. https://codereview.chromium.org/2513973002/diff/1/components/arc/video_accele... components/arc/video_accelerator/video_accelerator_struct_traits.h:30: } nit: empty line before to preserve symmetry.
Description was changed from ========== Use mojo typemaps to simplify GpuArcVideoService::BindDmabuf() BUG=665723' ========== to ========== Use mojo typemaps to simplify GpuArcVideoService::BindDmabuf() BUG=665723 ==========
https://codereview.chromium.org/2513973002/diff/1/components/arc/video_accele... File components/arc/video_accelerator/video_accelerator.h (right): https://codereview.chromium.org/2513973002/diff/1/components/arc/video_accele... components/arc/video_accelerator/video_accelerator.h:14: } On 2016/11/29 18:25:36, Luis Héctor Chávez wrote: > nit: empty line before to preserve symmetry. and add '// namespace arc'. https://codereview.chromium.org/2513973002/diff/1/components/arc/video_accele... File components/arc/video_accelerator/video_accelerator_struct_traits.cc (right): https://codereview.chromium.org/2513973002/diff/1/components/arc/video_accele... components/arc/video_accelerator/video_accelerator_struct_traits.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. The implementation doesn't make sense... I guess you accidentally added this file right? Please remove if so. https://codereview.chromium.org/2513973002/diff/1/components/arc/video_accele... File components/arc/video_accelerator/video_accelerator_struct_traits.h (right): https://codereview.chromium.org/2513973002/diff/1/components/arc/video_accele... components/arc/video_accelerator/video_accelerator_struct_traits.h:15: ::arc::ArcVideoAcceleratorDmabufPlane> { (here and elsewhere) The leading :: seems redundant. Since this function is not in chromeos::arc::, you can remove the leading ::. https://codereview.chromium.org/2513973002/diff/1/components/arc/video_accele... components/arc/video_accelerator/video_accelerator_struct_traits.h:30: } On 2016/11/29 18:25:36, Luis Héctor Chávez wrote: > nit: empty line before to preserve symmetry. same. // namespace mojo
The CQ bit was checked by yoshiki@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 checked by yoshiki@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: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
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.
Yusuke-san, Luis, PTAL. Thanks. https://codereview.chromium.org/2513973002/diff/1/chrome/gpu/gpu_arc_video_se... File chrome/gpu/gpu_arc_video_service.cc (right): https://codereview.chromium.org/2513973002/diff/1/chrome/gpu/gpu_arc_video_se... chrome/gpu/gpu_arc_video_service.cc:248: std::vector<::arc::ArcVideoAcceleratorDmabufPlane> planes(1); On 2016/11/29 18:25:35, Luis Héctor Chávez wrote: > You can maybe directly initialize the plane with an initializer list: > > std::vector<::arc::ArcVideoAcceleratorDmabufPlane> planes{ > {0, stride} > }; Done. https://codereview.chromium.org/2513973002/diff/1/chrome/gpu/gpu_arc_video_se... chrome/gpu/gpu_arc_video_service.cc:266: // TODO(yusukes): Use mojo typemaps to simplify the code. On 2016/11/29 18:25:35, Luis Héctor Chávez wrote: > you can now safely remove this, and the whole L267-L274 block if you also do the > comment in the typemap code. Done. https://codereview.chromium.org/2513973002/diff/1/components/arc/video_accele... File components/arc/video_accelerator/video_accelerator.h (right): https://codereview.chromium.org/2513973002/diff/1/components/arc/video_accele... components/arc/video_accelerator/video_accelerator.h:14: } On 2016/11/29 21:34:27, Yusuke Sato wrote: > On 2016/11/29 18:25:36, Luis Héctor Chávez wrote: > > nit: empty line before to preserve symmetry. > > and add '// namespace arc'. Done. https://codereview.chromium.org/2513973002/diff/1/components/arc/video_accele... File components/arc/video_accelerator/video_accelerator_struct_traits.cc (right): https://codereview.chromium.org/2513973002/diff/1/components/arc/video_accele... components/arc/video_accelerator/video_accelerator_struct_traits.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/11/29 21:34:28, Yusuke Sato wrote: > The implementation doesn't make sense... I guess you accidentally added this > file right? Please remove if so. Yes, this is mistake. Removed. https://codereview.chromium.org/2513973002/diff/1/components/arc/video_accele... File components/arc/video_accelerator/video_accelerator_struct_traits.h (right): https://codereview.chromium.org/2513973002/diff/1/components/arc/video_accele... components/arc/video_accelerator/video_accelerator_struct_traits.h:15: ::arc::ArcVideoAcceleratorDmabufPlane> { On 2016/11/29 21:34:28, Yusuke Sato wrote: > (here and elsewhere) > The leading :: seems redundant. Since this function is not in chromeos::arc::, > you can remove the leading ::. Done. https://codereview.chromium.org/2513973002/diff/1/components/arc/video_accele... components/arc/video_accelerator/video_accelerator_struct_traits.h:26: out->stride = data.stride(); On 2016/11/29 18:25:36, Luis Héctor Chávez wrote: > here you can check if either the stride or offset are invalid and log + return > false. Done. https://codereview.chromium.org/2513973002/diff/1/components/arc/video_accele... components/arc/video_accelerator/video_accelerator_struct_traits.h:30: } On 2016/11/29 21:34:28, Yusuke Sato wrote: > On 2016/11/29 18:25:36, Luis Héctor Chávez wrote: > > nit: empty line before to preserve symmetry. > > same. // namespace mojo Done.
lhchavez@chromium.org changed reviewers: + dcheng@chromium.org
lgtm Adding dcheng@ for the struct_traits.h https://codereview.chromium.org/2513973002/diff/40001/components/arc/video_ac... File components/arc/video_accelerator/video_accelerator_struct_traits.h (right): https://codereview.chromium.org/2513973002/diff/40001/components/arc/video_ac... components/arc/video_accelerator/video_accelerator_struct_traits.h:19: static uint32_t stride(const arc::ArcVideoAcceleratorDmabufPlane& r) { nit: blank line before, for consistency. https://codereview.chromium.org/2513973002/diff/40001/components/arc/video_ac... components/arc/video_accelerator/video_accelerator_struct_traits.h:36: } // namespace arc nit: namespace mojo
lgtm with comments. Defer to gpu and security owners. Thanks Yoshiki for working on this. https://codereview.chromium.org/2513973002/diff/40001/chrome/gpu/DEPS File chrome/gpu/DEPS (right): https://codereview.chromium.org/2513973002/diff/40001/chrome/gpu/DEPS#newcode1 chrome/gpu/DEPS:1: include_rules = [ Your CL is for allowing video_accelerator.mojom functions to use arc::ArcVideoAcceleratorDmabufPlane directly without having to convert from the corresponding arc::mojom:: type, right? Can you explain the motivation of the CL more clearly in the CL description? It's more than just BindDmaBuf(). https://codereview.chromium.org/2513973002/diff/40001/components/arc/video_ac... File components/arc/video_accelerator/video_accelerator_struct_traits.h (right): https://codereview.chromium.org/2513973002/diff/40001/components/arc/video_ac... components/arc/video_accelerator/video_accelerator_struct_traits.h:27: return false; qq: How is the false return handled? The original code in chrome/gpu/gpu_arc_video_service.cc for example does the following: 273 client_->OnError( 274 ::arc::mojom::VideoAcceleratorService::Result::INVALID_ARGUMENT); Does the same or similar happen? It might be a good idea to add some code comments for mojo typemap newbies like me.
https://codereview.chromium.org/2513973002/diff/40001/chrome/gpu/arc_gpu_vide... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/2513973002/diff/40001/chrome/gpu/arc_gpu_vide... chrome/gpu/arc_gpu_video_decode_accelerator.cc:213: current_size += plane.stride * rows; Btw, is it possible for this to overflow size_t before we ever perform the CheckedNumeric math? https://codereview.chromium.org/2513973002/diff/40001/components/arc/video_ac... File components/arc/video_accelerator/video_accelerator.h (right): https://codereview.chromium.org/2513973002/diff/40001/components/arc/video_ac... components/arc/video_accelerator/video_accelerator.h:11: int32_t offset; // in bytes Perhaps note in a comment why these are signed despite the fact they should only hold non-negative numbers. (IIRC, it's related to off_t being involved, but I don't remember the details anymore) https://codereview.chromium.org/2513973002/diff/40001/components/arc/video_ac... File components/arc/video_accelerator/video_accelerator_struct_traits.h (right): https://codereview.chromium.org/2513973002/diff/40001/components/arc/video_ac... components/arc/video_accelerator/video_accelerator_struct_traits.h:17: return r.offset; Perhaps DCHECK(r.offset > 0) https://codereview.chromium.org/2513973002/diff/40001/components/arc/video_ac... components/arc/video_accelerator/video_accelerator_struct_traits.h:20: return r.stride; Ditto https://codereview.chromium.org/2513973002/diff/40001/components/arc/video_ac... components/arc/video_accelerator/video_accelerator_struct_traits.h:25: if (data.offset() < 0 || data.stride() < 0) { Nit: please out-of-line this to a .cc file https://codereview.chromium.org/2513973002/diff/40001/components/arc/video_ac... components/arc/video_accelerator/video_accelerator_struct_traits.h:27: return false; On 2016/11/30 18:08:34, Yusuke Sato wrote: > qq: How is the false return handled? The original code in > chrome/gpu/gpu_arc_video_service.cc for example does the following: > > 273 client_->OnError( > 274 ::arc::mojom::VideoAcceleratorService::Result::INVALID_ARGUMENT); > > Does the same or similar happen? It might be a good idea to add some code > comments for mojo typemap newbies like me. When StructTraits::Read() returns false, the message fails validation and is never processed. I believe the message pipe is closed as well.
https://codereview.chromium.org/2513973002/diff/40001/components/arc/video_ac... File components/arc/video_accelerator/video_accelerator_struct_traits.h (right): https://codereview.chromium.org/2513973002/diff/40001/components/arc/video_ac... components/arc/video_accelerator/video_accelerator_struct_traits.h:27: return false; On 2016/12/01 01:30:50, dcheng wrote: > On 2016/11/30 18:08:34, Yusuke Sato wrote: > > qq: How is the false return handled? The original code in > > chrome/gpu/gpu_arc_video_service.cc for example does the following: > > > > 273 client_->OnError( > > 274 > ::arc::mojom::VideoAcceleratorService::Result::INVALID_ARGUMENT); > > > > Does the same or similar happen? It might be a good idea to add some code > > comments for mojo typemap newbies like me. > > When StructTraits::Read() returns false, the message fails validation and is > never processed. I believe the message pipe is closed as well. Got it, thanks!
yoshiki@chromium.org changed reviewers: + posciak@chromium.org
+posciak Powel, could you answer the 2 following questions, since you are the original author of the code? https://codereview.chromium.org/2513973002/diff/40001/chrome/gpu/arc_gpu_vide... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/2513973002/diff/40001/chrome/gpu/arc_gpu_vide... chrome/gpu/arc_gpu_video_decode_accelerator.cc:213: current_size += plane.stride * rows; On 2016/12/01 01:30:50, dcheng wrote: > Btw, is it possible for this to overflow size_t before we ever perform the > CheckedNumeric math? Powel, could you answer this question? https://codereview.chromium.org/2513973002/diff/40001/components/arc/video_ac... File components/arc/video_accelerator/video_accelerator.h (right): https://codereview.chromium.org/2513973002/diff/40001/components/arc/video_ac... components/arc/video_accelerator/video_accelerator.h:11: int32_t offset; // in bytes On 2016/12/01 01:30:50, dcheng wrote: > Perhaps note in a comment why these are signed despite the fact they should only > hold non-negative numbers. > > (IIRC, it's related to off_t being involved, but I don't remember the details > anymore) Powel, could you answer this question?
+posciak Powel, could you answer the 2 following questions, since you are the original author of the code?
https://codereview.chromium.org/2513973002/diff/40001/chrome/gpu/arc_gpu_vide... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/2513973002/diff/40001/chrome/gpu/arc_gpu_vide... chrome/gpu/arc_gpu_video_decode_accelerator.cc:213: current_size += plane.stride * rows; On 2016/12/02 08:31:43, yoshiki wrote: > On 2016/12/01 01:30:50, dcheng wrote: > > Btw, is it possible for this to overflow size_t before we ever perform the > > CheckedNumeric math? > > Powel, could you answer this question? Yes, it looks technically possible. https://codereview.chromium.org/2513973002/diff/40001/components/arc/video_ac... File components/arc/video_accelerator/video_accelerator.h (right): https://codereview.chromium.org/2513973002/diff/40001/components/arc/video_ac... components/arc/video_accelerator/video_accelerator.h:11: int32_t offset; // in bytes On 2016/12/02 08:31:43, yoshiki wrote: > On 2016/12/01 01:30:50, dcheng wrote: > > Perhaps note in a comment why these are signed despite the fact they should > only > > hold non-negative numbers. > > > > (IIRC, it's related to off_t being involved, but I don't remember the details > > anymore) > > Powel, could you answer this question? They are signed because client APIs use ints for these.
The CQ bit was checked by yoshiki@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...
Description was changed from ========== Use mojo typemaps to simplify GpuArcVideoService::BindDmabuf() BUG=665723 ========== to ========== Use mojo typemap to simplify the code using DmabufPlane This patch introduces a typemap for video_accelerator.mojom struct and allows to use arc::ArcVideoAcceleratorDmabufPlane directly without having to convert from the arc::mojom::DmabufPlane. BUG=665723 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dcheng@, I addressed your comments. PTAL again? Thanks. https://codereview.chromium.org/2513973002/diff/40001/chrome/gpu/DEPS File chrome/gpu/DEPS (right): https://codereview.chromium.org/2513973002/diff/40001/chrome/gpu/DEPS#newcode1 chrome/gpu/DEPS:1: include_rules = [ On 2016/11/30 18:08:34, Yusuke Sato wrote: > Your CL is for allowing video_accelerator.mojom functions to use > arc::ArcVideoAcceleratorDmabufPlane directly without having to convert from the > corresponding arc::mojom:: type, right? > > Can you explain the motivation of the CL more clearly in the CL description? > It's more than just BindDmaBuf(). Updated the CL description. https://codereview.chromium.org/2513973002/diff/40001/chrome/gpu/arc_gpu_vide... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/2513973002/diff/40001/chrome/gpu/arc_gpu_vide... chrome/gpu/arc_gpu_video_decode_accelerator.cc:213: current_size += plane.stride * rows; On 2016/12/06 07:08:38, Pawel Osciak wrote: > On 2016/12/02 08:31:43, yoshiki wrote: > > On 2016/12/01 01:30:50, dcheng wrote: > > > Btw, is it possible for this to overflow size_t before we ever perform the > > > CheckedNumeric math? > > > > Powel, could you answer this question? > > Yes, it looks technically possible. Done. I made this checked. https://codereview.chromium.org/2513973002/diff/40001/components/arc/video_ac... File components/arc/video_accelerator/video_accelerator.h (right): https://codereview.chromium.org/2513973002/diff/40001/components/arc/video_ac... components/arc/video_accelerator/video_accelerator.h:11: int32_t offset; // in bytes On 2016/12/06 07:08:38, Pawel Osciak wrote: > On 2016/12/02 08:31:43, yoshiki wrote: > > On 2016/12/01 01:30:50, dcheng wrote: > > > Perhaps note in a comment why these are signed despite the fact they should > > only > > > hold non-negative numbers. > > > > > > (IIRC, it's related to off_t being involved, but I don't remember the > details > > > anymore) > > > > Powel, could you answer this question? > > They are signed because client APIs use ints for these. Ok. Let me keep as-is. https://codereview.chromium.org/2513973002/diff/40001/components/arc/video_ac... File components/arc/video_accelerator/video_accelerator_struct_traits.h (right): https://codereview.chromium.org/2513973002/diff/40001/components/arc/video_ac... components/arc/video_accelerator/video_accelerator_struct_traits.h:17: return r.offset; On 2016/12/01 01:30:50, dcheng wrote: > Perhaps DCHECK(r.offset > 0) I believe this can be 0. Added DCHECK(r.offset >= 0). https://codereview.chromium.org/2513973002/diff/40001/components/arc/video_ac... components/arc/video_accelerator/video_accelerator_struct_traits.h:19: static uint32_t stride(const arc::ArcVideoAcceleratorDmabufPlane& r) { On 2016/11/30 17:57:01, Luis Héctor Chávez wrote: > nit: blank line before, for consistency. Done. https://codereview.chromium.org/2513973002/diff/40001/components/arc/video_ac... components/arc/video_accelerator/video_accelerator_struct_traits.h:20: return r.stride; On 2016/12/01 01:30:50, dcheng wrote: > Ditto Done. https://codereview.chromium.org/2513973002/diff/40001/components/arc/video_ac... components/arc/video_accelerator/video_accelerator_struct_traits.h:25: if (data.offset() < 0 || data.stride() < 0) { On 2016/12/01 01:30:50, dcheng wrote: > Nit: please out-of-line this to a .cc file Done. https://codereview.chromium.org/2513973002/diff/40001/components/arc/video_ac... components/arc/video_accelerator/video_accelerator_struct_traits.h:36: } // namespace arc On 2016/11/30 17:57:01, Luis Héctor Chávez wrote: > nit: namespace mojo Done.
lgtm
The CQ bit was checked by yoshiki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, yusukes@chromium.org Link to the patchset: https://codereview.chromium.org/2513973002/#ps60001 (title: "Addressed comments")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Powel, PTAL at chrome/gpu/* changes and approve the CL? Thanks.
posciak@, ping?
ping?
On 2016/12/13 21:17:04, Yusuke Sato (ooo Dec 16 to 31) wrote: > ping? I talked with posciak@ offline. Let me check the YT playback and the internal error count as he guided.
Description was changed from ========== Use mojo typemap to simplify the code using DmabufPlane This patch introduces a typemap for video_accelerator.mojom struct and allows to use arc::ArcVideoAcceleratorDmabufPlane directly without having to convert from the arc::mojom::DmabufPlane. BUG=665723 ========== to ========== Use mojo typemap to simplify the code using DmabufPlane This patch introduces a typemap for video_accelerator.mojom struct and allows to use arc::ArcVideoAcceleratorDmabufPlane directly without having to convert from the arc::mojom::DmabufPlane. BUG=665723 TEST=Ran YT and confirmed that histograms/Media.ArcGpuVideoDecodeAccelerator.InitializeResult is zero ==========
On 2016/12/14 01:51:42, yoshiki wrote: > On 2016/12/13 21:17:04, Yusuke Sato (ooo Dec 16 to 31) wrote: > > ping? > > I talked with posciak@ offline. Let me check the YT playback and the internal > error count as he guided. posciak@, could you review the code again? I confirmed that histograms/Media.ArcGpuVideoDecodeAccelerator.InitializeResult are zeros after playing some YT videos.
lgtm % nits https://codereview.chromium.org/2513973002/diff/60001/components/arc/video_ac... File components/arc/video_accelerator/video_accelerator.h (right): https://codereview.chromium.org/2513973002/diff/60001/components/arc/video_ac... components/arc/video_accelerator/video_accelerator.h:5: #ifndef COMPONENT_ARC_VIDEO_ACCELERATOR_VIDEO_ACCELERATOR_H_ s/COMPONENT/COMPONENTS/ https://codereview.chromium.org/2513973002/diff/60001/components/arc/video_ac... File components/arc/video_accelerator/video_accelerator_struct_traits.cc (right): https://codereview.chromium.org/2513973002/diff/60001/components/arc/video_ac... components/arc/video_accelerator/video_accelerator_struct_traits.cc:15: // Invalid offset/stride. Nit: this comment is probably unnecessary. https://codereview.chromium.org/2513973002/diff/60001/components/arc/video_ac... File components/arc/video_accelerator/video_accelerator_struct_traits.h (right): https://codereview.chromium.org/2513973002/diff/60001/components/arc/video_ac... components/arc/video_accelerator/video_accelerator_struct_traits.h:5: #ifndef COMPONENT_ARC_COMMON_VIDEO_ACCELERATOR_STRUCT_TRAITS_H_ s/COMPONENT_ARC_COMMON_VIDEO_ACCELERATOR_STRUCT_TRAITS_H_/COMPONENTS_ARC_VIDEO_ACCELERATOR_VIDEO_ACCELERATOR_STRUCT_TRAITS_H_/
The CQ bit was checked by yoshiki@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...
Patchset #4 (id:80001) has been deleted
The CQ bit was checked by yoshiki@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Description was changed from ========== Use mojo typemap to simplify the code using DmabufPlane This patch introduces a typemap for video_accelerator.mojom struct and allows to use arc::ArcVideoAcceleratorDmabufPlane directly without having to convert from the arc::mojom::DmabufPlane. BUG=665723 TEST=Ran YT and confirmed that histograms/Media.ArcGpuVideoDecodeAccelerator.InitializeResult is zero ========== to ========== Use mojo typemap to simplify the code using DmabufPlane This patch introduces a typemap for video_accelerator.mojom struct and allows to use arc::ArcVideoAcceleratorDmabufPlane directly without having to convert from the arc::mojom::DmabufPlane. BUG=665723 TEST=Ran YT and confirmed that histograms/Media.ArcGpuVideoDecodeAccelerator.InitializeResult is zero on chell and minnie ==========
The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run
Patchset #4 (id:100001) has been deleted
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 checked by yoshiki@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.
Thank you! Since I checked the InitializeResult value on minnie and chell as Powel guided offline (see the TEST filed for detail), let me submit. https://codereview.chromium.org/2513973002/diff/60001/components/arc/video_ac... File components/arc/video_accelerator/video_accelerator.h (right): https://codereview.chromium.org/2513973002/diff/60001/components/arc/video_ac... components/arc/video_accelerator/video_accelerator.h:5: #ifndef COMPONENT_ARC_VIDEO_ACCELERATOR_VIDEO_ACCELERATOR_H_ On 2016/12/14 08:53:14, Pawel Osciak wrote: > s/COMPONENT/COMPONENTS/ Done. https://codereview.chromium.org/2513973002/diff/60001/components/arc/video_ac... File components/arc/video_accelerator/video_accelerator_struct_traits.cc (right): https://codereview.chromium.org/2513973002/diff/60001/components/arc/video_ac... components/arc/video_accelerator/video_accelerator_struct_traits.cc:15: // Invalid offset/stride. On 2016/12/14 08:53:14, Pawel Osciak wrote: > Nit: this comment is probably unnecessary. Done. https://codereview.chromium.org/2513973002/diff/60001/components/arc/video_ac... File components/arc/video_accelerator/video_accelerator_struct_traits.h (right): https://codereview.chromium.org/2513973002/diff/60001/components/arc/video_ac... components/arc/video_accelerator/video_accelerator_struct_traits.h:5: #ifndef COMPONENT_ARC_COMMON_VIDEO_ACCELERATOR_STRUCT_TRAITS_H_ On 2016/12/14 08:53:14, Pawel Osciak wrote: > s/COMPONENT_ARC_COMMON_VIDEO_ACCELERATOR_STRUCT_TRAITS_H_/COMPONENTS_ARC_VIDEO_ACCELERATOR_VIDEO_ACCELERATOR_STRUCT_TRAITS_H_/ Done.
The CQ bit was checked by yoshiki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, yusukes@chromium.org, dcheng@chromium.org, posciak@chromium.org Link to the patchset: https://codereview.chromium.org/2513973002/#ps140001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1481725090517360,
"parent_rev": "fd462705097457eee26eae732a7cedb96b1e3b6d", "commit_rev":
"8ac7a33dc64c4e2850d3f3b58022f10699380662"}
Message was sent while issue was closed.
Description was changed from ========== Use mojo typemap to simplify the code using DmabufPlane This patch introduces a typemap for video_accelerator.mojom struct and allows to use arc::ArcVideoAcceleratorDmabufPlane directly without having to convert from the arc::mojom::DmabufPlane. BUG=665723 TEST=Ran YT and confirmed that histograms/Media.ArcGpuVideoDecodeAccelerator.InitializeResult is zero on chell and minnie ========== to ========== Use mojo typemap to simplify the code using DmabufPlane This patch introduces a typemap for video_accelerator.mojom struct and allows to use arc::ArcVideoAcceleratorDmabufPlane directly without having to convert from the arc::mojom::DmabufPlane. BUG=665723 TEST=Ran YT and confirmed that histograms/Media.ArcGpuVideoDecodeAccelerator.InitializeResult is zero on chell and minnie Review-Url: https://codereview.chromium.org/2513973002 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Use mojo typemap to simplify the code using DmabufPlane This patch introduces a typemap for video_accelerator.mojom struct and allows to use arc::ArcVideoAcceleratorDmabufPlane directly without having to convert from the arc::mojom::DmabufPlane. BUG=665723 TEST=Ran YT and confirmed that histograms/Media.ArcGpuVideoDecodeAccelerator.InitializeResult is zero on chell and minnie Review-Url: https://codereview.chromium.org/2513973002 ========== to ========== Use mojo typemap to simplify the code using DmabufPlane This patch introduces a typemap for video_accelerator.mojom struct and allows to use arc::ArcVideoAcceleratorDmabufPlane directly without having to convert from the arc::mojom::DmabufPlane. BUG=665723 TEST=Ran YT and confirmed that histograms/Media.ArcGpuVideoDecodeAccelerator.InitializeResult is zero on chell and minnie Committed: https://crrev.com/2f600a079c2fb7e8d9b37e6c246100d5ecdd59f7 Cr-Commit-Position: refs/heads/master@{#438512} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/2f600a079c2fb7e8d9b37e6c246100d5ecdd59f7 Cr-Commit-Position: refs/heads/master@{#438512} |
