Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2892863002:
ArcBridge: Add VideoEncodeAccelerator implementation. (Closed)
|
Created:
3 years, 7 months ago by Owen Lin Modified:
3 years, 5 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, oshima+watch_chromium.org, yusukes+watch_chromium.org, posciak+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, piman+watch_chromium.org, davemoore+watch_chromium.org, Ken Rockot(use gerrit already) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionArcBridge: Add VideoEncodeAccelerator implementation.
TEST=None
BUG=b:32692654
Review-Url: https://codereview.chromium.org/2892863002
Cr-Commit-Position: refs/heads/master@{#485178}
Committed: https://chromium.googlesource.com/chromium/src/+/c32115b7a1ea9b6a4b590fad5674d91e7ea69b78
Patch Set 1 #
Total comments: 7
Patch Set 2 : Rebase #
Total comments: 4
Patch Set 3 : Address Luis Review Comments #Patch Set 4 : Address Kcwu's comments #
Total comments: 12
Patch Set 5 : Address review comments from Luis #
Total comments: 2
Patch Set 6 : Address nit from Luis. #
Total comments: 9
Patch Set 7 : Address nits from Kcwu #
Total comments: 10
Patch Set 8 : Address nits from Pawel. #Patch Set 9 : Rebase #
Total comments: 17
Patch Set 10 : Address comments from dcheng #Patch Set 11 : Address dcheng's comments #Patch Set 12 : Change a missed LOG to DLOG. #Patch Set 13 : Rebase #Patch Set 14 : add missing public_dep to the typemap file. #Messages
Total messages: 61 (27 generated)
owenlin@chromium.org changed reviewers: + dcheng@chromium.org, lhchavez@google.com, posciak@chromium.org
PTAL. Thanks.
owenlin@chromium.org changed reviewers: + kcwu@chromium.org
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org - lhchavez@google.com
Adding the correct lhchavez. https://codereview.chromium.org/2892863002/diff/1/chrome/gpu/gpu_arc_video_en... File chrome/gpu/gpu_arc_video_encode_accelerator.cc (right): https://codereview.chromium.org/2892863002/diff/1/chrome/gpu/gpu_arc_video_en... chrome/gpu/gpu_arc_video_encode_accelerator.cc:24: media::VideoEncodeAccelerator::Error::value, \ Why not make a typemap for this? You also get compile-time checks for exhaustivity that way if you don't add a default case. https://codereview.chromium.org/2892863002/diff/1/chrome/gpu/gpu_arc_video_en... chrome/gpu/gpu_arc_video_encode_accelerator.cc:72: struct TypeConverter<arc::mojom::VideoEncodeProfilePtr, same here, TypeConverters are discouraged in new code. https://codereview.chromium.org/2892863002/diff/1/chrome/gpu/gpu_arc_video_en... chrome/gpu/gpu_arc_video_encode_accelerator.cc:133: gpu_preferences_)) nit: you cannot elide braces if the 'for' does not fit in one line. https://codereview.chromium.org/2892863002/diff/1/chrome/gpu/gpu_arc_video_en... File chrome/gpu/gpu_arc_video_encode_accelerator.h (right): https://codereview.chromium.org/2892863002/diff/1/chrome/gpu/gpu_arc_video_en... chrome/gpu/gpu_arc_video_encode_accelerator.h:21: // GpuArcVideoEncodeAccelerator manages life-cycle and IPC message translation nit: lifecycle is much more common (472 instances) than life-cycle (9) in src/chrome.
Description was changed from ========== Add gpu_arc_video_encode_accelerator for video encoding on ARC++. TEST=None BUG=b:32692654 ========== to ========== ArcBridge: Add VideoEncodeAccelerator implementation. TEST=None BUG=b:32692654 ==========
https://codereview.chromium.org/2892863002/diff/20001/chrome/gpu/chrome_conte... File chrome/gpu/chrome_content_gpu_client.h (right): https://codereview.chromium.org/2892863002/diff/20001/chrome/gpu/chrome_conte... chrome/gpu/chrome_content_gpu_client.h:16: #include "chrome/gpu/gpu_arc_video_decode_accelerator.h" why the code is asymmetric for encode and decode? could this line be replaced by #include "components/arc/common/video_decode_accelerator.mojom.h" ? https://codereview.chromium.org/2892863002/diff/20001/chrome/gpu/gpu_arc_vide... File chrome/gpu/gpu_arc_video_encode_accelerator.h (right): https://codereview.chromium.org/2892863002/diff/20001/chrome/gpu/gpu_arc_vide... chrome/gpu/gpu_arc_video_encode_accelerator.h:22: // for ArcVideoAccelerator. VideoEncodeAccelerator?
https://codereview.chromium.org/2892863002/diff/1/chrome/gpu/gpu_arc_video_en... File chrome/gpu/gpu_arc_video_encode_accelerator.cc (right): https://codereview.chromium.org/2892863002/diff/1/chrome/gpu/gpu_arc_video_en... chrome/gpu/gpu_arc_video_encode_accelerator.cc:24: media::VideoEncodeAccelerator::Error::value, \ On 2017/05/19 15:14:44, Luis Héctor Chávez wrote: > Why not make a typemap for this? You also get compile-time checks for > exhaustivity that way if you don't add a default case. Done. https://codereview.chromium.org/2892863002/diff/1/chrome/gpu/gpu_arc_video_en... chrome/gpu/gpu_arc_video_encode_accelerator.cc:72: struct TypeConverter<arc::mojom::VideoEncodeProfilePtr, On 2017/05/19 15:14:44, Luis Héctor Chávez wrote: > same here, TypeConverters are discouraged in new code. Done. https://codereview.chromium.org/2892863002/diff/1/chrome/gpu/gpu_arc_video_en... chrome/gpu/gpu_arc_video_encode_accelerator.cc:133: gpu_preferences_)) On 2017/05/19 15:14:44, Luis Héctor Chávez wrote: > nit: you cannot elide braces if the 'for' does not fit in one line. Code is modified.
https://codereview.chromium.org/2892863002/diff/20001/chrome/gpu/chrome_conte... File chrome/gpu/chrome_content_gpu_client.h (right): https://codereview.chromium.org/2892863002/diff/20001/chrome/gpu/chrome_conte... chrome/gpu/chrome_content_gpu_client.h:16: #include "chrome/gpu/gpu_arc_video_decode_accelerator.h" On 2017/06/03 10:58:28, kcwu wrote: > why the code is asymmetric for encode and decode? > could this line be replaced by > #include "components/arc/common/video_decode_accelerator.mojom.h" > ? Done. https://codereview.chromium.org/2892863002/diff/20001/chrome/gpu/gpu_arc_vide... File chrome/gpu/gpu_arc_video_encode_accelerator.h (right): https://codereview.chromium.org/2892863002/diff/20001/chrome/gpu/gpu_arc_vide... chrome/gpu/gpu_arc_video_encode_accelerator.h:22: // for ArcVideoAccelerator. On 2017/06/03 10:58:28, kcwu wrote: > VideoEncodeAccelerator? Updated. I also remove the last paragraph as it seems outdated.
https://codereview.chromium.org/2892863002/diff/60001/chrome/gpu/gpu_arc_vide... File chrome/gpu/gpu_arc_video_encode_accelerator.cc (right): https://codereview.chromium.org/2892863002/diff/60001/chrome/gpu/gpu_arc_vide... chrome/gpu/gpu_arc_video_encode_accelerator.cc:60: std::move(media::GpuVideoEncodeAcceleratorFactory::GetSupportedProfiles( nit: this std::move() is not needed since it's not an lvalue. https://codereview.chromium.org/2892863002/diff/60001/chrome/gpu/gpu_arc_vide... chrome/gpu/gpu_arc_video_encode_accelerator.cc:87: DLOG(ERROR) << "failed to create vea."; nit: s/vea/Accelerator/, for consistency with the rest of the file. https://codereview.chromium.org/2892863002/diff/60001/chrome/gpu/gpu_arc_vide... chrome/gpu/gpu_arc_video_encode_accelerator.cc:112: frame = media::VideoFrame::CreateEOSFrame(); nit: use guard clause pattern: if (planes.empty()) { frame = media::VideoFrame::CreateEOSFrame(); accelerator_->Encode(frame, force_keyframe); return; } // rest (maybe even skip declaring |frame| and just make it one line: accelerator_->Encode(media::VideoFrame::CreateEOSFrame(), force_keyframe); ) https://codereview.chromium.org/2892863002/diff/60001/chrome/gpu/gpu_arc_vide... File chrome/gpu/gpu_arc_video_encode_accelerator.h (right): https://codereview.chromium.org/2892863002/diff/60001/chrome/gpu/gpu_arc_vide... chrome/gpu/gpu_arc_video_encode_accelerator.h:70: uint32_t framerate) override; Can you remove blank lines in L58, 63, 68? That way it's consistent with the style in L38-45. https://codereview.chromium.org/2892863002/diff/60001/chrome/gpu/gpu_arc_vide... chrome/gpu/gpu_arc_video_encode_accelerator.h:72: base::ScopedFD UnwrapFdFromMojoHandle(mojo::ScopedHandle handle); Can you add a small comment?
https://codereview.chromium.org/2892863002/diff/60001/chrome/gpu/gpu_arc_vide... File chrome/gpu/gpu_arc_video_encode_accelerator.cc (right): https://codereview.chromium.org/2892863002/diff/60001/chrome/gpu/gpu_arc_vide... chrome/gpu/gpu_arc_video_encode_accelerator.cc:144: DLOG(ERROR) << "Could not map memroy"; nit: s/memroy/memory/
Patchset #5 (id:80001) has been deleted
https://codereview.chromium.org/2892863002/diff/60001/chrome/gpu/gpu_arc_vide... File chrome/gpu/gpu_arc_video_encode_accelerator.cc (right): https://codereview.chromium.org/2892863002/diff/60001/chrome/gpu/gpu_arc_vide... chrome/gpu/gpu_arc_video_encode_accelerator.cc:60: std::move(media::GpuVideoEncodeAcceleratorFactory::GetSupportedProfiles( On 2017/06/07 16:02:36, Luis Héctor Chávez wrote: > nit: this std::move() is not needed since it's not an lvalue. I see. Thanks. https://codereview.chromium.org/2892863002/diff/60001/chrome/gpu/gpu_arc_vide... chrome/gpu/gpu_arc_video_encode_accelerator.cc:87: DLOG(ERROR) << "failed to create vea."; On 2017/06/07 16:02:36, Luis Héctor Chávez wrote: > nit: s/vea/Accelerator/, for consistency with the rest of the file. Done. https://codereview.chromium.org/2892863002/diff/60001/chrome/gpu/gpu_arc_vide... chrome/gpu/gpu_arc_video_encode_accelerator.cc:112: frame = media::VideoFrame::CreateEOSFrame(); On 2017/06/07 16:02:36, Luis Héctor Chávez wrote: > nit: > > use guard clause pattern: > > if (planes.empty()) { > frame = media::VideoFrame::CreateEOSFrame(); > accelerator_->Encode(frame, force_keyframe); > return; > } > > // rest > > (maybe even skip declaring |frame| and just make it one line: > > accelerator_->Encode(media::VideoFrame::CreateEOSFrame(), > force_keyframe); > > ) Done. https://codereview.chromium.org/2892863002/diff/60001/chrome/gpu/gpu_arc_vide... chrome/gpu/gpu_arc_video_encode_accelerator.cc:144: DLOG(ERROR) << "Could not map memroy"; On 2017/06/07 16:17:36, Luis Héctor Chávez wrote: > nit: s/memroy/memory/ Done. https://codereview.chromium.org/2892863002/diff/60001/chrome/gpu/gpu_arc_vide... File chrome/gpu/gpu_arc_video_encode_accelerator.h (right): https://codereview.chromium.org/2892863002/diff/60001/chrome/gpu/gpu_arc_vide... chrome/gpu/gpu_arc_video_encode_accelerator.h:70: uint32_t framerate) override; On 2017/06/07 16:02:37, Luis Héctor Chávez wrote: > Can you remove blank lines in L58, 63, 68? That way it's consistent with the > style in L38-45. Done. https://codereview.chromium.org/2892863002/diff/60001/chrome/gpu/gpu_arc_vide... chrome/gpu/gpu_arc_video_encode_accelerator.h:72: base::ScopedFD UnwrapFdFromMojoHandle(mojo::ScopedHandle handle); On 2017/06/07 16:02:36, Luis Héctor Chávez wrote: > Can you add a small comment? Done.
lgtm with nit https://codereview.chromium.org/2892863002/diff/100001/chrome/gpu/gpu_arc_vid... File chrome/gpu/gpu_arc_video_encode_accelerator.h (right): https://codereview.chromium.org/2892863002/diff/100001/chrome/gpu/gpu_arc_vid... chrome/gpu/gpu_arc_video_encode_accelerator.h:69: // At errors, returns an invalid base::ScopedFD and notifies client about nit: "If an error is encountered, it returns..."
https://codereview.chromium.org/2892863002/diff/100001/chrome/gpu/gpu_arc_vid... File chrome/gpu/gpu_arc_video_encode_accelerator.h (right): https://codereview.chromium.org/2892863002/diff/100001/chrome/gpu/gpu_arc_vid... chrome/gpu/gpu_arc_video_encode_accelerator.h:69: // At errors, returns an invalid base::ScopedFD and notifies client about On 2017/06/08 15:00:43, Luis Héctor Chávez wrote: > nit: "If an error is encountered, it returns..." Done. Thanks.
lgtm; nits https://codereview.chromium.org/2892863002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/video/gpu_arc_video_service_host.cc (right): https://codereview.chromium.org/2892863002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/video/gpu_arc_video_service_host.cc:57: base::Bind(&ConnectToEncodeAcceleratorOnIOThread, BindOnce https://codereview.chromium.org/2892863002/diff/120001/components/arc/BUILD.gn File components/arc/BUILD.gn (right): https://codereview.chromium.org/2892863002/diff/120001/components/arc/BUILD.g... components/arc/BUILD.gn:62: "video_accelerator/video_accelerator_struct_traits.cc", Are these forgot to rename? https://codereview.chromium.org/2892863002/diff/120001/components/arc/common/... File components/arc/common/video_encode_accelerator.typemap (right): https://codereview.chromium.org/2892863002/diff/120001/components/arc/common/... components/arc/common/video_encode_accelerator.typemap:1: # Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2892863002/diff/120001/components/arc/video_a... File components/arc/video_accelerator/video_encode_accelerator_struct_traits.h (right): https://codereview.chromium.org/2892863002/diff/120001/components/arc/video_a... components/arc/video_accelerator/video_encode_accelerator_struct_traits.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017
https://codereview.chromium.org/2892863002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/video/gpu_arc_video_service_host.cc (right): https://codereview.chromium.org/2892863002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/video/gpu_arc_video_service_host.cc:57: base::Bind(&ConnectToEncodeAcceleratorOnIOThread, On 2017/06/09 06:30:24, kcwu wrote: > BindOnce Done. https://codereview.chromium.org/2892863002/diff/120001/components/arc/BUILD.gn File components/arc/BUILD.gn (right): https://codereview.chromium.org/2892863002/diff/120001/components/arc/BUILD.g... components/arc/BUILD.gn:62: "video_accelerator/video_accelerator_struct_traits.cc", On 2017/06/09 06:30:24, kcwu wrote: > Are these forgot to rename? There are common struct_traits for both decoders and encoder. So, video_accelerator works. But maybe it's better to rename those files to video_common_struct_traits.h. WDYT? Anyway, will do that in another CL. https://codereview.chromium.org/2892863002/diff/120001/components/arc/common/... File components/arc/common/video_encode_accelerator.typemap (right): https://codereview.chromium.org/2892863002/diff/120001/components/arc/common/... components/arc/common/video_encode_accelerator.typemap:1: # Copyright 2016 The Chromium Authors. All rights reserved. On 2017/06/09 06:30:24, kcwu wrote: > 2017 Done. https://codereview.chromium.org/2892863002/diff/120001/components/arc/video_a... File components/arc/video_accelerator/video_encode_accelerator_struct_traits.h (right): https://codereview.chromium.org/2892863002/diff/120001/components/arc/video_a... components/arc/video_accelerator/video_encode_accelerator_struct_traits.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/06/09 06:30:24, kcwu wrote: > 2017 Done.
https://codereview.chromium.org/2892863002/diff/120001/components/arc/BUILD.gn File components/arc/BUILD.gn (right): https://codereview.chromium.org/2892863002/diff/120001/components/arc/BUILD.g... components/arc/BUILD.gn:62: "video_accelerator/video_accelerator_struct_traits.cc", On 2017/06/12 06:34:13, Owen Lin wrote: > On 2017/06/09 06:30:24, kcwu wrote: > > Are these forgot to rename? > > There are common struct_traits for both decoders and encoder. So, > video_accelerator works. But maybe it's better to rename those files to > video_common_struct_traits.h. > > WDYT? Anyway, will do that in another CL. Oh, then no need to rename them.
https://codereview.chromium.org/2892863002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/video/gpu_arc_video_service_host.cc (right): https://codereview.chromium.org/2892863002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/video/gpu_arc_video_service_host.cc:34: void ConnectToEncodeAcceleratorOnIOThread( s/ConnectToEncodeAcceleratorOnIOThread/ConnectToVideoEncodeAcceleratorOnIOThread/ ? https://codereview.chromium.org/2892863002/diff/140001/chrome/gpu/gpu_arc_vid... File chrome/gpu/gpu_arc_video_encode_accelerator.cc (right): https://codereview.chromium.org/2892863002/diff/140001/chrome/gpu/gpu_arc_vid... chrome/gpu/gpu_arc_video_encode_accelerator.cc:31: DVLOG(2) << "RequireBitstreamBuffers(input_count=" << input_count Not sure about your preference, but perhaps a macro like DVLOGF (and other *F macros) similar to https://cs.chromium.org/chromium/src/media/gpu/v4l2_slice_video_decode_accele... would be convenient? https://codereview.chromium.org/2892863002/diff/140001/chrome/gpu/gpu_arc_vid... chrome/gpu/gpu_arc_video_encode_accelerator.cc:74: DLOG(ERROR) << "Invalid client"; Perhaps LOG(ERROR) https://codereview.chromium.org/2892863002/diff/140001/chrome/gpu/gpu_arc_vid... chrome/gpu/gpu_arc_video_encode_accelerator.cc:86: DLOG(ERROR) << "failed to create Accelerator."; Failed to create a VideoEncodeAccelerator. https://codereview.chromium.org/2892863002/diff/140001/chrome/gpu/gpu_arc_vid... chrome/gpu/gpu_arc_video_encode_accelerator.cc:100: std::vector<::arc::VideoFramePlane> planes, Could this be a const&?
Forgot to say, lgtm % nits above. Thanks!
https://codereview.chromium.org/2892863002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/video/gpu_arc_video_service_host.cc (right): https://codereview.chromium.org/2892863002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/video/gpu_arc_video_service_host.cc:34: void ConnectToEncodeAcceleratorOnIOThread( On 2017/06/14 03:57:38, Pawel Osciak wrote: > s/ConnectToEncodeAcceleratorOnIOThread/ConnectToVideoEncodeAcceleratorOnIOThread/ > ? Done. https://codereview.chromium.org/2892863002/diff/140001/chrome/gpu/gpu_arc_vid... File chrome/gpu/gpu_arc_video_encode_accelerator.cc (right): https://codereview.chromium.org/2892863002/diff/140001/chrome/gpu/gpu_arc_vid... chrome/gpu/gpu_arc_video_encode_accelerator.cc:31: DVLOG(2) << "RequireBitstreamBuffers(input_count=" << input_count On 2017/06/14 03:57:38, Pawel Osciak wrote: > Not sure about your preference, but perhaps a macro like DVLOGF (and other *F > macros) similar to > https://cs.chromium.org/chromium/src/media/gpu/v4l2_slice_video_decode_accele... > would be convenient? That's nice. https://codereview.chromium.org/2892863002/diff/140001/chrome/gpu/gpu_arc_vid... chrome/gpu/gpu_arc_video_encode_accelerator.cc:74: DLOG(ERROR) << "Invalid client"; On 2017/06/14 03:57:38, Pawel Osciak wrote: > Perhaps LOG(ERROR) Done. https://codereview.chromium.org/2892863002/diff/140001/chrome/gpu/gpu_arc_vid... chrome/gpu/gpu_arc_video_encode_accelerator.cc:86: DLOG(ERROR) << "failed to create Accelerator."; On 2017/06/14 03:57:38, Pawel Osciak wrote: > Failed to create a VideoEncodeAccelerator. Done. https://codereview.chromium.org/2892863002/diff/140001/chrome/gpu/gpu_arc_vid... chrome/gpu/gpu_arc_video_encode_accelerator.cc:100: std::vector<::arc::VideoFramePlane> planes, On 2017/06/14 03:57:38, Pawel Osciak wrote: > Could this be a const&? The interface is defined by mojom. It is not const because we declare VideoFramePlane as "move only" in the typemap.
owenlin@chromium.org changed reviewers: + xhwang@chromium.org
Hi Xiaohan, Please help review the DEPS changes. Thank you.
DEPS change lgtm
Hi Daniel, Please help review the following files. Thank you. chrome/browser/chrome_content_gpu_manifest_overlay.json components/arc/common/video_encode_accelerator.typemap components/arc/video_accelerator/video_encode_accelerator_struct_traits.cc components/arc/video_accelerator/video_encode_accelerator_struct_traits.h
sorry for the belated review =( https://codereview.chromium.org/2892863002/diff/180001/chrome/gpu/gpu_arc_vid... File chrome/gpu/gpu_arc_video_encode_accelerator.cc (right): https://codereview.chromium.org/2892863002/diff/180001/chrome/gpu/gpu_arc_vid... chrome/gpu/gpu_arc_video_encode_accelerator.cc:75: if (!client) { Does Mojo actually allow a null interface pointer to passed if the parameter wasn't marked optional? https://codereview.chromium.org/2892863002/diff/180001/chrome/gpu/gpu_arc_vid... chrome/gpu/gpu_arc_video_encode_accelerator.cc:81: input_pixel_format_ = static_cast<media::VideoPixelFormat>(input_format); Do we need these casts? Seems like they're the same time Nit: also spell VideoPixelFormat consistently, sometimes it's spelled without media::, but other times, we include the media:: qualifier. https://codereview.chromium.org/2892863002/diff/180001/chrome/gpu/gpu_arc_vid... chrome/gpu/gpu_arc_video_encode_accelerator.cc:85: static_cast<media::VideoCodecProfile>(output_profile), initial_bitrate, Similarly, I'm not sure why this requires a cast. https://codereview.chromium.org/2892863002/diff/180001/chrome/gpu/gpu_arc_vid... chrome/gpu/gpu_arc_video_encode_accelerator.cc:88: LOG(ERROR) << "Failed to create a VideoEncodeAccelerator."; Would personally suggest DLOG for everything that uses LOG here. https://codereview.chromium.org/2892863002/diff/180001/chrome/gpu/gpu_arc_vid... chrome/gpu/gpu_arc_video_encode_accelerator.cc:186: 0u, guid); What is the behavior if we just pass 0 here? Does the Map() later just fail? https://codereview.chromium.org/2892863002/diff/180001/components/arc/video_a... File components/arc/video_accelerator/video_encode_accelerator_struct_traits.cc (right): https://codereview.chromium.org/2892863002/diff/180001/components/arc/video_a... components/arc/video_accelerator/video_encode_accelerator_struct_traits.cc:69: LOG(ERROR) << "Unknown VideoPixelFormat: " << input; Nit: DLOG https://codereview.chromium.org/2892863002/diff/180001/components/arc/video_a... File components/arc/video_accelerator/video_encode_accelerator_struct_traits.h (right): https://codereview.chromium.org/2892863002/diff/180001/components/arc/video_a... components/arc/video_accelerator/video_encode_accelerator_struct_traits.h:46: static gfx::Size max_resolution( Nit: return non-trivial types by const ref
https://codereview.chromium.org/2892863002/diff/180001/chrome/gpu/gpu_arc_vid... File chrome/gpu/gpu_arc_video_encode_accelerator.cc (right): https://codereview.chromium.org/2892863002/diff/180001/chrome/gpu/gpu_arc_vid... chrome/gpu/gpu_arc_video_encode_accelerator.cc:75: if (!client) { On 2017/06/21 09:42:29, dcheng wrote: > Does Mojo actually allow a null interface pointer to passed if the parameter > wasn't marked optional? You're right. I tested it, it failed in the IPC message validation phase. Code removed. https://codereview.chromium.org/2892863002/diff/180001/chrome/gpu/gpu_arc_vid... chrome/gpu/gpu_arc_video_encode_accelerator.cc:81: input_pixel_format_ = static_cast<media::VideoPixelFormat>(input_format); On 2017/06/21 09:42:28, dcheng wrote: > Do we need these casts? Seems like they're the same time > > Nit: also spell VideoPixelFormat consistently, sometimes it's spelled without > media::, but other times, we include the media:: qualifier. Removed. It should be removed after we type mapping the enums. https://codereview.chromium.org/2892863002/diff/180001/chrome/gpu/gpu_arc_vid... chrome/gpu/gpu_arc_video_encode_accelerator.cc:85: static_cast<media::VideoCodecProfile>(output_profile), initial_bitrate, On 2017/06/21 09:42:29, dcheng wrote: > Similarly, I'm not sure why this requires a cast. Done. https://codereview.chromium.org/2892863002/diff/180001/chrome/gpu/gpu_arc_vid... chrome/gpu/gpu_arc_video_encode_accelerator.cc:88: LOG(ERROR) << "Failed to create a VideoEncodeAccelerator."; On 2017/06/21 09:42:29, dcheng wrote: > Would personally suggest DLOG for everything that uses LOG here. Those messages should only be logged when errors happen, when it happens, the info would be helpful. Why would you suggest not to use LOG? https://codereview.chromium.org/2892863002/diff/180001/chrome/gpu/gpu_arc_vid... chrome/gpu/gpu_arc_video_encode_accelerator.cc:186: 0u, guid); On 2017/06/21 09:42:29, dcheng wrote: > What is the behavior if we just pass 0 here? Does the Map() later just fail? AFAIK, this argument is not used yet. rockot@ should know the details. (also see crbug.com/713763)
https://codereview.chromium.org/2892863002/diff/180001/chrome/gpu/gpu_arc_vid... File chrome/gpu/gpu_arc_video_encode_accelerator.cc (right): https://codereview.chromium.org/2892863002/diff/180001/chrome/gpu/gpu_arc_vid... chrome/gpu/gpu_arc_video_encode_accelerator.cc:88: LOG(ERROR) << "Failed to create a VideoEncodeAccelerator."; On 2017/06/22 07:43:14, Owen Lin wrote: > On 2017/06/21 09:42:29, dcheng wrote: > > Would personally suggest DLOG for everything that uses LOG here. > > Those messages should only be logged when errors happen, when it happens, the > info would be helpful. > Why would you suggest not to use LOG? Right, but is it useful to non-developers? Using LOG instead of DLOG means these strings are shipped to all CrOS users. https://codereview.chromium.org/2892863002/diff/180001/components/arc/video_a... File components/arc/video_accelerator/video_encode_accelerator_struct_traits.h (right): https://codereview.chromium.org/2892863002/diff/180001/components/arc/video_a... components/arc/video_accelerator/video_encode_accelerator_struct_traits.h:46: static gfx::Size max_resolution( On 2017/06/21 09:42:29, dcheng wrote: > Nit: return non-trivial types by const ref I think this might have been missed in the latest patchset?
https://codereview.chromium.org/2892863002/diff/180001/chrome/gpu/gpu_arc_vid... File chrome/gpu/gpu_arc_video_encode_accelerator.cc (right): https://codereview.chromium.org/2892863002/diff/180001/chrome/gpu/gpu_arc_vid... chrome/gpu/gpu_arc_video_encode_accelerator.cc:88: LOG(ERROR) << "Failed to create a VideoEncodeAccelerator."; On 2017/06/22 20:05:13, dcheng wrote: > On 2017/06/22 07:43:14, Owen Lin wrote: > > On 2017/06/21 09:42:29, dcheng wrote: > > > Would personally suggest DLOG for everything that uses LOG here. > > > > Those messages should only be logged when errors happen, when it happens, the > > info would be helpful. > > Why would you suggest not to use LOG? > > Right, but is it useful to non-developers? Using LOG instead of DLOG means these > strings are shipped to all CrOS users. OK. I got your point. We would like to use LOG simply because we don't run debug build in the lab. https://codereview.chromium.org/2892863002/diff/180001/components/arc/video_a... File components/arc/video_accelerator/video_encode_accelerator_struct_traits.h (right): https://codereview.chromium.org/2892863002/diff/180001/components/arc/video_a... components/arc/video_accelerator/video_encode_accelerator_struct_traits.h:46: static gfx::Size max_resolution( On 2017/06/22 20:05:13, dcheng wrote: > On 2017/06/21 09:42:29, dcheng wrote: > > Nit: return non-trivial types by const ref > > I think this might have been missed in the latest patchset? Sorry, I missed that. Done.
https://codereview.chromium.org/2892863002/diff/180001/chrome/gpu/gpu_arc_vid... File chrome/gpu/gpu_arc_video_encode_accelerator.cc (right): https://codereview.chromium.org/2892863002/diff/180001/chrome/gpu/gpu_arc_vid... chrome/gpu/gpu_arc_video_encode_accelerator.cc:88: LOG(ERROR) << "Failed to create a VideoEncodeAccelerator."; On 2017/06/23 01:37:54, Owen Lin wrote: > On 2017/06/22 20:05:13, dcheng wrote: > > On 2017/06/22 07:43:14, Owen Lin wrote: > > > On 2017/06/21 09:42:29, dcheng wrote: > > > > Would personally suggest DLOG for everything that uses LOG here. > > > > > > Those messages should only be logged when errors happen, when it happens, > the > > > info would be helpful. > > > Why would you suggest not to use LOG? > > > > Right, but is it useful to non-developers? Using LOG instead of DLOG means > these > > strings are shipped to all CrOS users. > > OK. I got your point. We would like to use LOG simply because we don't run debug > build in the lab. I don't think we should be bloating official builds with logging strings that are only used for internal debugging. Among other things, this contradicts the general advice about logging. See https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... If this is being used in labs, it shouldn't be that hard to make a build which enables these logs and test on that machine/config.
On 2017/06/23 07:38:44, dcheng wrote: > https://codereview.chromium.org/2892863002/diff/180001/chrome/gpu/gpu_arc_vid... > File chrome/gpu/gpu_arc_video_encode_accelerator.cc (right): > > https://codereview.chromium.org/2892863002/diff/180001/chrome/gpu/gpu_arc_vid... > chrome/gpu/gpu_arc_video_encode_accelerator.cc:88: LOG(ERROR) << "Failed to > create a VideoEncodeAccelerator."; > On 2017/06/23 01:37:54, Owen Lin wrote: > > On 2017/06/22 20:05:13, dcheng wrote: > > > On 2017/06/22 07:43:14, Owen Lin wrote: > > > > On 2017/06/21 09:42:29, dcheng wrote: > > > > > Would personally suggest DLOG for everything that uses LOG here. > > > > > > > > Those messages should only be logged when errors happen, when it happens, > > the > > > > info would be helpful. > > > > Why would you suggest not to use LOG? > > > > > > Right, but is it useful to non-developers? Using LOG instead of DLOG means > > these > > > strings are shipped to all CrOS users. > > > > OK. I got your point. We would like to use LOG simply because we don't run > debug > > build in the lab. > > I don't think we should be bloating official builds with logging strings that > are only used for internal debugging. Among other things, this contradicts the > general advice about logging. See > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > If this is being used in labs, it shouldn't be that hard to make a build which > enables these logs and test on that machine/config. I agree with you. However, enabling dcheck/dlog for chrome would be a big challenge. We will start with unittests. Sorry for the late update. Please have another looks. Thanks.
dcheng@google.com changed reviewers: + dcheng@google.com
ipc lgtm
lgtm from the right account this time
The CQ bit was checked by owenlin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, kcwu@chromium.org, posciak@chromium.org, xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/2892863002/#ps240001 (title: "Change a missed LOG to DLOG.")
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...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by owenlin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, dcheng@chromium.org, dcheng@google.com, kcwu@chromium.org, xhwang@chromium.org, posciak@chromium.org Link to the patchset: https://codereview.chromium.org/2892863002/#ps260001 (title: "Rebase")
The CQ bit was unchecked by owenlin@chromium.org
Patchset #13 (id:260001) has been deleted
The CQ bit was checked by owenlin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, dcheng@chromium.org, dcheng@google.com, kcwu@chromium.org, xhwang@chromium.org, posciak@chromium.org Link to the patchset: https://codereview.chromium.org/2892863002/#ps280001 (title: "Rebase")
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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #14 (id:300001) has been deleted
Patchset #14 (id:320001) has been deleted
The CQ bit was checked by owenlin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, dcheng@chromium.org, dcheng@google.com, kcwu@chromium.org, xhwang@chromium.org, posciak@chromium.org Link to the patchset: https://codereview.chromium.org/2892863002/#ps340001 (title: "add missing public_dep to the typemap file.")
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": 340001, "attempt_start_ts": 1499653255927950,
"parent_rev": "8c0eda14fc8be0293528bb3f9a3a401b318c960e", "commit_rev":
"c32115b7a1ea9b6a4b590fad5674d91e7ea69b78"}
Message was sent while issue was closed.
Description was changed from ========== ArcBridge: Add VideoEncodeAccelerator implementation. TEST=None BUG=b:32692654 ========== to ========== ArcBridge: Add VideoEncodeAccelerator implementation. TEST=None BUG=b:32692654 Review-Url: https://codereview.chromium.org/2892863002 Cr-Commit-Position: refs/heads/master@{#485178} Committed: https://chromium.googlesource.com/chromium/src/+/c32115b7a1ea9b6a4b590fad5674... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:340001) as https://chromium.googlesource.com/chromium/src/+/c32115b7a1ea9b6a4b590fad5674... |
