Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1549473002:
Add ArcGpuVideoDecodeAccelerator. (Closed)
|
Created:
5 years ago by Owen Lin Modified:
4 years, 8 months ago Reviewers:
Ken Russell (switch to Gerrit), jochen (gone - plz use gerrit), cpu_(ooo_6.6-7.5), jam, kcwu, wuchengli, Pawel Osciak CC:
darin-cc_chromium.org, mcasas+watch_chromium.org, piman+watch_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd ArcGpuVideoDecodeAccelerator.
This class runs in the GPU process. It takes video decoding requests
from the ARC and talk with real ArcVDA implementations to
decode the video.
BUG=b/25829285
Test=Compile the code on veyron_minnie.
Committed: https://crrev.com/76e5c4476a8a2cfba57c2e03216f1ce6dc7762c0
Cr-Commit-Position: refs/heads/master@{#388525}
Patch Set 1 #
Total comments: 26
Patch Set 2 : Address kcwu's comments and change to use new API #Patch Set 3 : Not ready yet #
Total comments: 73
Patch Set 4 : address posciak's comments #Patch Set 5 : rebase #
Total comments: 19
Patch Set 6 : address kcwu's comments and bug fixes #Patch Set 7 : move code from content/ to /chrome #
Total comments: 56
Patch Set 8 : address posciak's comments #
Total comments: 32
Patch Set 9 : address kcwu's comments #
Total comments: 4
Patch Set 10 : address kcwu's comments #Patch Set 11 : add OWNERS file #
Total comments: 10
Patch Set 12 : Add check for |vda_| in IPC functions. #
Total comments: 26
Patch Set 13 : don't use |arc_client_| when |vda| is null. #Patch Set 14 : Change ArcVideoAccelerator::Reset to asynchronous. #
Total comments: 52
Patch Set 15 : address Pawel's comments #
Total comments: 12
Patch Set 16 : address Pawel's comments #
Total comments: 6
Patch Set 17 : rebased #Patch Set 18 : address Pawel's nits #Patch Set 19 : address kcwu's comments #Patch Set 20 : rebase #Patch Set 21 : fix compiling errors #Patch Set 22 : add out-of-line ctor/dtor #
Messages
Total messages: 66 (21 generated)
owenlin@chromium.org changed reviewers: + kcwu@chromium.org, posciak@chromium.org
https://codereview.chromium.org/1549473002/diff/1/content/common/gpu/media/ar... File content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1549473002/diff/1/content/common/gpu/media/ar... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:60: PortInfo* port_info = &port_info_[port]; validate value of |port| https://codereview.chromium.org/1549473002/diff/1/content/common/gpu/media/ar... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:95: NOTREACHED(); return EINVAL; |port| may come from less trusted client. https://codereview.chromium.org/1549473002/diff/1/content/common/gpu/media/ar... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:104: std::vector<BufferInfo>* buffers = &port_info_[port].buffers; How about: auto& buffers = port_info_[port].buffers; return index >= buffers.size() ? nullptr : &buffers[index]; https://codereview.chromium.org/1549473002/diff/1/content/common/gpu/media/ar... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:115: PortInfo* port_info = &port_info_[port]; Value of |port| need to validate. Please either 1. Check the value of |port| before use, or 2. Move this after GetBufferInfo since GetBufferInfo will check for us. https://codereview.chromium.org/1549473002/diff/1/content/common/gpu/media/ar... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:140: PortInfo* port_info = &port_info_[port]; Ditto, check value of |port| https://codereview.chromium.org/1549473002/diff/1/content/common/gpu/media/ar... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:185: base::SharedMemoryHandle shared_memory_handle(buffer_info->handle.get(), when will the handle be closed? https://codereview.chromium.org/1549473002/diff/1/content/common/gpu/media/ar... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:277: static ArcVideoAccelerator::Error convertErrorCode( s/convert/Convert/ https://codereview.chromium.org/1549473002/diff/1/content/common/gpu/media/ar... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:320: static const size_t kMaxInputBufferDataSize = 128; s/static// local constant doesn't need static storage. https://codereview.chromium.org/1549473002/diff/1/content/common/gpu/media/ar... File content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.h (right): https://codereview.chromium.org/1549473002/diff/1/content/common/gpu/media/ar... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.h:24: // Implementation of the VideoAccelerator interface. s/ArcVideoAccelerator/ https://codereview.chromium.org/1549473002/diff/1/content/common/gpu/media/ar... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.h:27: remove blank lines between ArcVideoAccelerator methods. https://codereview.chromium.org/1549473002/diff/1/content/common/gpu/media/ar... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.h:60: // Enumerations indicates the state of an buffer. s/indicates/indicate/ https://codereview.chromium.org/1549473002/diff/1/content/common/gpu/media/ar... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.h:62: OWNED_BY_CLIENT, // The buffer is sent to and used by |client_|. How about add a new state, something like NO_BINDED or NO_BUFFER? https://codereview.chromium.org/1549473002/diff/1/content/common/gpu/media/ar... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.h:114: // Helper function to Send the end-of-stream output buffer if Put these member functions before member variables. https://codereview.chromium.org/1549473002/diff/1/content/content_common.gypi File content/content_common.gypi (right): https://codereview.chromium.org/1549473002/diff/1/content/content_common.gypi... content/content_common.gypi:879: 'common/gpu/media/arc/arc_video_accelerator.h', sort by alphabet.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/1549473002/diff/1/content/common/gpu/media/ar... File content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1549473002/diff/1/content/common/gpu/media/ar... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:60: PortInfo* port_info = &port_info_[port]; On 2015/12/23 06:25:43, kcwu wrote: > validate value of |port| Done. https://codereview.chromium.org/1549473002/diff/1/content/common/gpu/media/ar... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:95: NOTREACHED(); On 2015/12/23 06:25:43, kcwu wrote: > return EINVAL; > |port| may come from less trusted client. Done. https://codereview.chromium.org/1549473002/diff/1/content/common/gpu/media/ar... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:104: std::vector<BufferInfo>* buffers = &port_info_[port].buffers; On 2015/12/23 06:25:43, kcwu wrote: > How about: > auto& buffers = port_info_[port].buffers; > return index >= buffers.size() ? nullptr : &buffers[index]; Done. https://codereview.chromium.org/1549473002/diff/1/content/common/gpu/media/ar... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:115: PortInfo* port_info = &port_info_[port]; On 2015/12/23 06:25:43, kcwu wrote: > Value of |port| need to validate. > Please either > 1. Check the value of |port| before use, or > 2. Move this after GetBufferInfo since GetBufferInfo will check for us. Done. https://codereview.chromium.org/1549473002/diff/1/content/common/gpu/media/ar... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:140: PortInfo* port_info = &port_info_[port]; On 2015/12/23 06:25:43, kcwu wrote: > Ditto, check value of |port| Done. https://codereview.chromium.org/1549473002/diff/1/content/common/gpu/media/ar... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:185: base::SharedMemoryHandle shared_memory_handle(buffer_info->handle.get(), On 2015/12/23 06:25:43, kcwu wrote: > when will the handle be closed? For now, it will be close when this ArcGVDA destroyed. In the future, it will be closed when we assigned a new set of Pictures. https://codereview.chromium.org/1549473002/diff/1/content/common/gpu/media/ar... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:277: static ArcVideoAccelerator::Error convertErrorCode( On 2015/12/23 06:25:43, kcwu wrote: > s/convert/Convert/ Done. https://codereview.chromium.org/1549473002/diff/1/content/common/gpu/media/ar... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:320: static const size_t kMaxInputBufferDataSize = 128; On 2015/12/23 06:25:43, kcwu wrote: > s/static// > local constant doesn't need static storage. Done. https://codereview.chromium.org/1549473002/diff/1/content/common/gpu/media/ar... File content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.h (right): https://codereview.chromium.org/1549473002/diff/1/content/common/gpu/media/ar... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.h:24: // Implementation of the VideoAccelerator interface. On 2015/12/23 06:25:43, kcwu wrote: > s/ArcVideoAccelerator/ Done. https://codereview.chromium.org/1549473002/diff/1/content/common/gpu/media/ar... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.h:27: On 2015/12/23 06:25:43, kcwu wrote: > remove blank lines between ArcVideoAccelerator methods. Done. https://codereview.chromium.org/1549473002/diff/1/content/common/gpu/media/ar... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.h:62: OWNED_BY_CLIENT, // The buffer is sent to and used by |client_|. On 2015/12/23 06:25:43, kcwu wrote: > How about add a new state, something like NO_BINDED or NO_BUFFER? Done. https://codereview.chromium.org/1549473002/diff/1/content/common/gpu/media/ar... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.h:114: // Helper function to Send the end-of-stream output buffer if On 2015/12/23 06:25:43, kcwu wrote: > Put these member functions before member variables. Done.
https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... File content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:30: status_t ArcGpuVideoDecodeAccelerator::Initialize( Please use a ThreadChecker to make sure methods are run on the correct thread. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:34: return EINVAL; I'm not seeing any other values apart from EINVAL and 0 returned from status_t methods. Perhaps we could consider using Chrome style and return bools? https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:84: vda_ = nullptr; // TODO(owenlin): Change to real implementation. Do we need this and l.80? vda_ can be constructed without knowing the profile. Perhaps we should consider constructing it earlier. We should have similar code as in GVDA to select the correct VDA to construct, and (perhaps eventually) share it. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:92: vda_->Initialize(config, this); This may fail, we should handle the return value. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:131: PortInfo* port_info = &port_info_[port]; It's not straightforward that we are depending on GetBufferInfo to validate port. Could we instead validate memory type in GetBufferInfo and remove l.131-135? https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:136: if (buffer_info->state != NOT_BOUND || I think we can drop NOT_BOUND state. VDA has to verify it has a valid handle anyway before using the buffer. Similarly, it has to check if ReusePictureBuffer is called on a picture_id that is already owned by it. This and with my comment about flush below, I think we could remove buffer states entirely? https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:153: BufferInfo* buffer_info = GetBufferInfo(port, index); Could we unify the code with the other Bind? It seems that the only difference is the Import path at the end. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:181: arc_client_->OnError(INVALID_ARGUMENT); It would be great to remove status returns and use this everywhere instead... https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:202: bitstream_buffer_serial_ = 0; Perhaps something like https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/gpu_... ? https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:209: buffer_info->state = OWNED_BY_VDA; I'm wondering if we could merge OWNED_BY_US and OWNED_BY_VDA to have one state less. Instead of marking buffers with OWNED_BY_US, we could just push them onto a queue: in the class: std::queue<Picture> pictures_pending_eos_; then: PictureReady() { if (picture.picture_buffer_id == -1) { pictures_pending_eos_.push(picture); } else { // ... arc_client->OnBufferDone(...); } } UseBuffer() { // ... SendEosIfNeededOrReusePicture(picture); } SendEosIfNeededOrReusePicture(Picture picture) { if (pending_eos_output_buffer_) { pictures_pending_eos_.push(picture); TryFinishEos(); } else { vda->ReusePictureBuffer(picture...); } } NotifyFlushDone() { pending_eos_output_buffer_ = true; TryFinishEos(); } TryFinishEos() { DCHECK(pending_eos_output_buffer_); if (pictures_pending_eos_.empty()) { // will have to send EOS on next UseBuffer() return; } while (!pictures_pending_eos_.empty()) { Picture picture = pictures_pending_eos_.front(); pictures_pending_eos_.pop(); if (!eos_sent) { // send EOS in picture } else { vda->ReusePictureBuffer(picture); } } pending_eos_output_buffer_ = false; } https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:211: } else { Perhaps reversing the if() and removing else would make it a bit easier to read: if (metadata.bytes_used == 0) { buffer_info->state = OWNED_BY_CLIENT; arc_client_->OnBufferDone(PORT_INPUT, index, BufferMetadata()); return; } // here handle normal case https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:241: // TODO(owenlin): Think if we need to handle the pixel format. Please use VDA::GetOutputFormat() here. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:243: VideoFormat video_format; How are we handling image_size ? https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:253: CHECK_NE(output_buffer, nullptr); I know this shouldn't happen, but I'm wondering if we should crash ourselves anyway. I'd prefer NotifyError(PLATFORM_FAILURE). https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:261: GetInputRecord(picture.bitstream_buffer_id(), &index, &metadata.timestamp); Could we use picture.picture_buffer_id as a key to get index? Then we wouldn't fail returning the correct buffer index, even if metadata was not found for some reason. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:263: arc_client_->OnBufferDone(PORT_OUTPUT, picture.picture_buffer_id(), should this be s/picture.picture_buffer_id()/index/ ? https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:274: CHECK(buffer_info); Perhaps NotifyError(PLATFORM_FAILURE). https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:276: arc_client_->OnBufferDone(PORT_INPUT, bitstream_buffer_id, BufferMetadata()); should this be s/bitstream_buffer_id/index/ ? https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:295: #define CASE(t) \ To be honest, I'd prefer not having this macro please. It makes code shorter, but a bit more difficult to understand/read for others. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:336: const size_t kMaxInputBufferDataSize = 128; In GVD we can get a theoretically unspecified number of input buffers, so we can't put a cap on the number of input buffers waiting to be decoded. However here we cannot get more buffers than we have valid index values. Would it be possible to instead just have a fixed vector (size of allowed index values for inputs?) of these? https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... File content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.h (right): https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. We'll have to s/2015/2016/ at some point I think. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.h:19: class ArcGpuVideoDecodeAccelerator Please document what this class is for. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.h:47: void DismissPictureBuffer(int32_t picture_buffer); override https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.h:63: struct BufferInfo { For bitstream buffers (inputs), I think we could store the handle in InputRecord. For picture buffers/outputs, we don't store the handle and don't use offset and length. Also, if at some point we start supporting shared memory for outputs, we will pass the handle/other details directly to Import...() like we do right now with dmabuf fds. So with the removal of state as proposed in my later comments, I think we could remove this struct completely? As for buffer index lookup in NotifyEndOfBitstreamBuffer and PictureReady: I think we could use InputRecord to get index of an input buffer back. For PictureReady I think we have to be mapping via picture_id, not bitstream_buffer_id, due to the special case I'm mentioning in later comments. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... File content/common/gpu/media/arc/arc_video_accelerator.h (right): https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_video_accelerator.h:30: MEMORY_SHARED_MEMORY = 1, Is there any specific reason to start with 1 here? https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_video_accelerator.h:45: uint32_t pixel_format; // the v4l2 fourcc format could we call this fourcc and remove v4l2 mention please? https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_video_accelerator.h:46: uint32_t image_size; Is this used? https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_video_accelerator.h:90: // Called when a buffer with the specified @index and @port has been |index| |port| https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_video_accelerator.h:93: // be used. s/used/consumed by the client/ https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_video_accelerator.h:103: virtual status_t Initialize(DeviceType device, Client* client); = 0 here and in methods below please https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_video_accelerator.h:108: virtual status_t BindSharedBuffer(PortType port, BindSharedMemory https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_video_accelerator.h:110: int ashmem_fd, Could we use base::SharedMemoryHandle here? https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_video_accelerator.h:117: virtual status_t BindGraphicBuffer(PortType port, BindDmabuf https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_video_accelerator.h:134: // accessed by the accelerator and there won't be more callbacks. Does this mean we require the VDA to return all buffers? VDA will not return outputs on vda->Reset(), only inputs. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_video_accelerator.h:138: virtual status_t SetBufferFormat(PortType port, const BufferFormat& format); Sorry, I think I'm forgetting why we needed this call, for inputs I think we could pass the format to Initialize directly, for outputs we actually get the format from VDA in ProvidePictureBuffers... https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_video_accelerator.h:143: // DISALLOW_EVIL_CONSTRUCTORS(ArcVideoAccelerator); I think we should remove this?
PTAL. Thanks. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... File content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:30: status_t ArcGpuVideoDecodeAccelerator::Initialize( On 2016/01/07 09:22:35, Pawel Osciak wrote: > Please use a ThreadChecker to make sure methods are run on the correct thread. Done. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:34: return EINVAL; On 2016/01/07 09:22:35, Pawel Osciak wrote: > I'm not seeing any other values apart from EINVAL and 0 returned from status_t > methods. Perhaps we could consider using Chrome style and return bools? Done. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:34: return EINVAL; On 2016/01/07 09:22:35, Pawel Osciak wrote: > I'm not seeing any other values apart from EINVAL and 0 returned from status_t > methods. Perhaps we could consider using Chrome style and return bools? Done. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:84: vda_ = nullptr; // TODO(owenlin): Change to real implementation. On 2016/01/07 09:22:35, Pawel Osciak wrote: > Do we need this and l.80? vda_ can be constructed without knowing the profile. > Perhaps we should consider constructing it earlier. > We should have similar code as in GVDA to select the correct VDA to construct, > and (perhaps eventually) share it. Move it to Initialize. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:92: vda_->Initialize(config, this); On 2016/01/07 09:22:35, Pawel Osciak wrote: > This may fail, we should handle the return value. Done. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:131: PortInfo* port_info = &port_info_[port]; On 2016/01/07 09:22:36, Pawel Osciak wrote: > It's not straightforward that we are depending on GetBufferInfo to validate > port. Could we instead validate memory type in GetBufferInfo and remove > l.131-135? But we call GetBufferInfo a lot. In most cases, there is no need to check the memory type. How about a new "ValidatePortAndIndex()" function to do the validation explicitly? https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:136: if (buffer_info->state != NOT_BOUND || On 2016/01/07 09:22:35, Pawel Osciak wrote: > I think we can drop NOT_BOUND state. VDA has to verify it has a valid handle > anyway before using the buffer. Similarly, it has to check if ReusePictureBuffer > is called on a picture_id that is already owned by it. This and with my comment > about flush below, I think we could remove buffer states entirely? Done. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:153: BufferInfo* buffer_info = GetBufferInfo(port, index); On 2016/01/07 09:22:35, Pawel Osciak wrote: > Could we unify the code with the other Bind? It seems that the only difference > is the Import path at the end. After we remove the check for buffer's state the only thing is to check port's memory type. I don't think we need another function to do that. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:181: arc_client_->OnError(INVALID_ARGUMENT); On 2016/01/07 09:22:35, Pawel Osciak wrote: > It would be great to remove status returns and use this everywhere instead... I think it's the difference between the design of Android and Chromium. Most of the OMX functions are synchronized. I am not sure about Codec 2.0, maybe we can gave Lajos some suggestions. For simplicity, I can only make UseBuffer() async now. We can try to make more function asynchronized when we remove AVA.h. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:202: bitstream_buffer_serial_ = 0; On 2016/01/07 09:22:35, Pawel Osciak wrote: > Perhaps something like > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/gpu_... > ? Done. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:209: buffer_info->state = OWNED_BY_VDA; On 2016/01/07 09:22:35, Pawel Osciak wrote: > I'm wondering if we could merge OWNED_BY_US and OWNED_BY_VDA to have one state > less. Instead of marking buffers with OWNED_BY_US, we could just push them onto > a queue: > > in the class: > > std::queue<Picture> pictures_pending_eos_; > > > then: > > PictureReady() { > if (picture.picture_buffer_id == -1) { > pictures_pending_eos_.push(picture); > } else { > // ... > arc_client->OnBufferDone(...); > } > } > > UseBuffer() { > // ... > SendEosIfNeededOrReusePicture(picture); > } > > SendEosIfNeededOrReusePicture(Picture picture) { > if (pending_eos_output_buffer_) { > pictures_pending_eos_.push(picture); > TryFinishEos(); > } else { > vda->ReusePictureBuffer(picture...); > } > } > > NotifyFlushDone() { > pending_eos_output_buffer_ = true; > TryFinishEos(); > } > > TryFinishEos() { > DCHECK(pending_eos_output_buffer_); > if (pictures_pending_eos_.empty()) { > // will have to send EOS on next UseBuffer() > return; > } > > while (!pictures_pending_eos_.empty()) { > Picture picture = pictures_pending_eos_.front(); > pictures_pending_eos_.pop(); > if (!eos_sent) { > // send EOS in picture > } else { > vda->ReusePictureBuffer(picture); > } > } > > pending_eos_output_buffer_ = false; > } Done. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:211: } else { On 2016/01/07 09:22:35, Pawel Osciak wrote: > Perhaps reversing the if() and removing else would make it a bit easier to read: > > if (metadata.bytes_used == 0) { > buffer_info->state = OWNED_BY_CLIENT; > arc_client_->OnBufferDone(PORT_INPUT, index, BufferMetadata()); > return; > } > > // here handle normal case Done. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:241: // TODO(owenlin): Think if we need to handle the pixel format. On 2016/01/07 09:22:35, Pawel Osciak wrote: > Please use VDA::GetOutputFormat() here. TODO added. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:243: VideoFormat video_format; On 2016/01/07 09:22:35, Pawel Osciak wrote: > How are we handling image_size ? We use it to request buffers from NativeWindow. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:253: CHECK_NE(output_buffer, nullptr); On 2016/01/07 09:22:35, Pawel Osciak wrote: > I know this shouldn't happen, but I'm wondering if we should crash ourselves > anyway. I'd prefer NotifyError(PLATFORM_FAILURE). Done. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:261: GetInputRecord(picture.bitstream_buffer_id(), &index, &metadata.timestamp); On 2016/01/07 09:22:36, Pawel Osciak wrote: > Could we use picture.picture_buffer_id as a key to get index? Then we wouldn't > fail returning the correct buffer index, even if metadata was not found for some > reason. Sorry, I didn't get you. Here we don't use the |index| at all. What' we need is the timestamp. And it was originally associated with the bitstream buffer. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:263: arc_client_->OnBufferDone(PORT_OUTPUT, picture.picture_buffer_id(), On 2016/01/07 09:22:35, Pawel Osciak wrote: > should this be s/picture.picture_buffer_id()/index/ ? No, GetInputRecord() returns the information about the bitstream. It's the buffer index we used for the bitstream. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:274: CHECK(buffer_info); On 2016/01/07 09:22:35, Pawel Osciak wrote: > Perhaps NotifyError(PLATFORM_FAILURE). Done. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:276: arc_client_->OnBufferDone(PORT_INPUT, bitstream_buffer_id, BufferMetadata()); On 2016/01/07 09:22:35, Pawel Osciak wrote: > should this be s/bitstream_buffer_id/index/ ? Yes, it is. Thanks. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:295: #define CASE(t) \ On 2016/01/07 09:22:35, Pawel Osciak wrote: > To be honest, I'd prefer not having this macro please. It makes code shorter, > but a bit more difficult to understand/read for others. Done. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:336: const size_t kMaxInputBufferDataSize = 128; On 2016/01/07 09:22:35, Pawel Osciak wrote: > In GVD we can get a theoretically unspecified number of input buffers, so we > can't put a cap on the number of input buffers waiting to be decoded. However > here we cannot get more buffers than we have valid index values. Would it be > possible to instead just have a fixed vector (size of allowed index values for > inputs?) of these? I think you're right, I will use the BufferInfo for this. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... File content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.h (right): https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/01/07 09:22:36, Pawel Osciak wrote: > We'll have to s/2015/2016/ at some point I think. Done. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.h:19: class ArcGpuVideoDecodeAccelerator On 2016/01/07 09:22:36, Pawel Osciak wrote: > Please document what this class is for. Done. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.h:47: void DismissPictureBuffer(int32_t picture_buffer); On 2016/01/07 09:22:36, Pawel Osciak wrote: > override Done. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.h:63: struct BufferInfo { On 2016/01/07 09:22:36, Pawel Osciak wrote: > For bitstream buffers (inputs), I think we could store the handle in > InputRecord. For picture buffers/outputs, we don't store the handle and don't > use offset and length. Also, if at some point we start supporting shared memory > for outputs, we will pass the handle/other details directly to Import...() like > we do right now with dmabuf fds. > > So with the removal of state as proposed in my later comments, I think we could > remove this struct completely? > > As for buffer index lookup in NotifyEndOfBitstreamBuffer and PictureReady: > I think we could use InputRecord to get index of an input buffer back. For > PictureReady I think we have to be mapping via picture_id, not > bitstream_buffer_id, due to the special case I'm mentioning in later comments. Done. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... File content/common/gpu/media/arc/arc_video_accelerator.h (right): https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_video_accelerator.h:30: MEMORY_SHARED_MEMORY = 1, On 2016/01/07 09:22:36, Pawel Osciak wrote: > Is there any specific reason to start with 1 here? I don't really remember why. Let's start with 0 just like others. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_video_accelerator.h:45: uint32_t pixel_format; // the v4l2 fourcc format On 2016/01/07 09:22:36, Pawel Osciak wrote: > could we call this fourcc and remove v4l2 mention please? Done. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_video_accelerator.h:46: uint32_t image_size; On 2016/01/07 09:22:36, Pawel Osciak wrote: > Is this used? Actually not. Removed. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_video_accelerator.h:90: // Called when a buffer with the specified @index and @port has been On 2016/01/07 09:22:36, Pawel Osciak wrote: > |index| |port| Done. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_video_accelerator.h:93: // be used. On 2016/01/07 09:22:36, Pawel Osciak wrote: > s/used/consumed by the client/ Done. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_video_accelerator.h:103: virtual status_t Initialize(DeviceType device, Client* client); On 2016/01/07 09:22:36, Pawel Osciak wrote: > = 0 here and in methods below please Done. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_video_accelerator.h:108: virtual status_t BindSharedBuffer(PortType port, On 2016/01/07 09:22:36, Pawel Osciak wrote: > BindSharedMemory Done. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_video_accelerator.h:110: int ashmem_fd, On 2016/01/07 09:22:36, Pawel Osciak wrote: > Could we use base::SharedMemoryHandle here? No, it's a common interface used on both Android and Chromium side. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_video_accelerator.h:117: virtual status_t BindGraphicBuffer(PortType port, On 2016/01/07 09:22:36, Pawel Osciak wrote: > BindDmabuf Done. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_video_accelerator.h:134: // accessed by the accelerator and there won't be more callbacks. On 2016/01/07 09:22:36, Pawel Osciak wrote: > Does this mean we require the VDA to return all buffers? VDA will not return > outputs on vda->Reset(), only inputs. I have modified the code in ArcCodec to allow VDA keep those buffers. Comment modified. https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_video_accelerator.h:138: virtual status_t SetBufferFormat(PortType port, const BufferFormat& format); On 2016/01/07 09:22:36, Pawel Osciak wrote: > Sorry, I think I'm forgetting why we needed this call, for inputs I think we > could pass the format to Initialize directly, for outputs we actually get the > format from VDA in ProvidePictureBuffers... For output, we only use it to set the MemoryType. On Android side, there are two phases related, We call Initialize() in onAllocateComponent(), but call SetBufferFormat() in onConfigureComponent(). One thing we can do is delaying Initialize() and call it in onConfigureComponent(). How do you think? https://codereview.chromium.org/1549473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/arc/arc_video_accelerator.h:143: // DISALLOW_EVIL_CONSTRUCTORS(ArcVideoAccelerator); On 2016/01/07 09:22:36, Pawel Osciak wrote: > I think we should remove this? Done.
https://codereview.chromium.org/1549473002/diff/120001/content/common/gpu/med... File content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1549473002/diff/120001/content/common/gpu/med... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:46: for (int32_t id = 0, n = *count; id < n; ++id) { how about just for (int32_t id = 0; id < *count; ++id) ? I think compiler can do the right optimization. https://codereview.chromium.org/1549473002/diff/120001/content/common/gpu/med... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:48: buffers.push_back(media::PictureBuffer(id, coded_size_, 0)); need to make sure the value of *count is not too big. Otherwise push_back may lead to OOM. https://codereview.chromium.org/1549473002/diff/120001/content/common/gpu/med... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:70: DVLOG(1) << "Only SharedMemory is supported for input buffers"; I feel most of DVLOG(1) in this file should be DLOG(ERROR) https://codereview.chromium.org/1549473002/diff/120001/content/common/gpu/med... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:110: bool ArcGpuVideoDecodeAccelerator::ValidatePort(PortType port) { Move these two helpers below or above, don't mix with ArcVideoAccelerator::* methods. https://codereview.chromium.org/1549473002/diff/120001/content/common/gpu/med... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:157: // Make sure we will close the file descriptor. this comment should be in front of line 155 https://codereview.chromium.org/1549473002/diff/120001/content/common/gpu/med... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:207: } break; https://codereview.chromium.org/1549473002/diff/120001/content/common/gpu/med... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:221: vda_->Reset(); check vda_ is not null. https://codereview.chromium.org/1549473002/diff/120001/content/common/gpu/med... File content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.h (right): https://codereview.chromium.org/1549473002/diff/120001/content/common/gpu/med... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.h:93: uint32_t FindInputRecordByBitstreamBufferId(int32_t bitstream_buffer_id); move this to line 85. To keep methods in front of variables. https://codereview.chromium.org/1549473002/diff/120001/content/content_common... File content/content_common.gypi (right): https://codereview.chromium.org/1549473002/diff/120001/content/content_common... content/content_common.gypi:882: 'common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc', .cc before .h
Description was changed from ========== Add ArcGpuVideoDecodeAccelerator. This class runs in the GPU process. It takes video decoding requests from the ARC container and talk with real ArcVDA implementations to decode the video. BUG=b/25829285 Test=Compile the code on veyron_minnie. ========== to ========== Add ArcGpuVideoDecodeAccelerator. This class runs in the GPU process. It takes video decoding requests from the ARC and talk with real ArcVDA implementations to decode the video. BUG=b/25829285 Test=Compile the code on veyron_minnie. ==========
Patchset #6 (id:140001) has been deleted
https://codereview.chromium.org/1549473002/diff/120001/content/common/gpu/med... File content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1549473002/diff/120001/content/common/gpu/med... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:46: for (int32_t id = 0, n = *count; id < n; ++id) { On 2016/01/20 09:11:05, kcwu wrote: > how about just > for (int32_t id = 0; id < *count; ++id) ? > I think compiler can do the right optimization. Also prevent comparing signed and unsigned numbers. https://codereview.chromium.org/1549473002/diff/120001/content/common/gpu/med... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:48: buffers.push_back(media::PictureBuffer(id, coded_size_, 0)); On 2016/01/20 09:11:05, kcwu wrote: > need to make sure the value of *count is not too big. Otherwise push_back may > lead to OOM. Done. https://codereview.chromium.org/1549473002/diff/120001/content/common/gpu/med... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:70: DVLOG(1) << "Only SharedMemory is supported for input buffers"; On 2016/01/20 09:11:05, kcwu wrote: > I feel most of DVLOG(1) in this file should be DLOG(ERROR) Done. https://codereview.chromium.org/1549473002/diff/120001/content/common/gpu/med... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:110: bool ArcGpuVideoDecodeAccelerator::ValidatePort(PortType port) { On 2016/01/20 09:11:04, kcwu wrote: > Move these two helpers below or above, don't mix with ArcVideoAccelerator::* > methods. Done. https://codereview.chromium.org/1549473002/diff/120001/content/common/gpu/med... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:157: // Make sure we will close the file descriptor. On 2016/01/20 09:11:05, kcwu wrote: > this comment should be in front of line 155 Done. https://codereview.chromium.org/1549473002/diff/120001/content/common/gpu/med... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:207: } On 2016/01/20 09:11:05, kcwu wrote: > break; Thanks. I spend few minutes on this. :p https://codereview.chromium.org/1549473002/diff/120001/content/common/gpu/med... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:221: vda_->Reset(); On 2016/01/20 09:11:05, kcwu wrote: > check vda_ is not null. Done. https://codereview.chromium.org/1549473002/diff/120001/content/common/gpu/med... File content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.h (right): https://codereview.chromium.org/1549473002/diff/120001/content/common/gpu/med... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.h:93: uint32_t FindInputRecordByBitstreamBufferId(int32_t bitstream_buffer_id); On 2016/01/20 09:11:05, kcwu wrote: > move this to line 85. > To keep methods in front of variables. Done. https://codereview.chromium.org/1549473002/diff/120001/content/content_common... File content/content_common.gypi (right): https://codereview.chromium.org/1549473002/diff/120001/content/content_common... content/content_common.gypi:882: 'common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc', On 2016/01/20 09:11:05, kcwu wrote: > .cc before .h Done.
Patchset #7 (id:180001) has been deleted
https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:72: size_t* count) { We are not assigning to count, do we need it to be an in/out value? I believe this is only used as a final count and not negotiable anymore? We get min. number of buffers in ProvidePictureBuffers, send it via OnOutpufFormatChanged, and then receive final count via SetBufferCount? https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:78: DVLOG(1) << "Must call setBufferFormat before setBufferCount"; VDA is now created in Initialize()... But ideally we could possibly remove the need for SetBufferFormat() (please see other comments). https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:81: if (port == PORT_OUTPUT) { We should probably check we don't have any existing buffers already and refuse this otherwise. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:91: if (port == PORT_INPUT) { Is it ok to allow SetBufferCount at any time for inputs? For example, if we are still decoding, but client calls SetBufferCount for INPUT, we drop the inputs, but the VDA still has them and may return them to us. So we could signal we are done for buffers that were dropped. Perhaps better not to allow SetBufferCount at all if we already have buffers? https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:98: bool ArcGpuVideoDecodeAccelerator::SetBufferFormat(PortType port, IIRC, we could not change format and memory type after initial setup. If so, perhaps we should pass formats to AVA::Initialize() and remove this method? https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:117: config.profile = media::H264PROFILE_MAIN; On second thought, could we pass the exact profile and use it here? A VDA may support a subset of profiles for a given codec and the VDA interface already handles it. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:128: if (!vda_->Initialize(config, this)) { We should probably make sure vda_ is not null, otherwise the client may crash us. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:137: DVLOG(1) << "Only DMA buffer is supported for output buffers"; s/DMA buffer/dmabuf/ https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:172: size_t offset, off_t ? https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:182: if (port != PORT_INPUT) { Should we instead check against port_info->memory_type for given port? https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:202: if (port != PORT_OUTPUT) { Here as well, should we instead check against port_info->memory_type for given port? https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:231: if (metadata.bytes_used == 0) { Could we handle this case in VDAs? We already do I believe. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:235: if (metadata.bytes_used > input_info->length) { I think this also should be validated in the VDA, as it has to do it anyway? https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:293: // TODO(owenlin): How to get visible size? Do we need it at this time for the client? https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:303: // no-op I'm wondering if we should still clean up here, i.e. close the fd, mark the buffer as dismissed and return it to the client. Next time we get it from the client we could just ignore it. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:310: if (!ValidatePortAndIndex(PORT_OUTPUT, picture.picture_buffer_id())) { Do we need to validate PR(), since it's coming from the VDA? https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:397: void ArcGpuVideoDecodeAccelerator::SetInputRecord(int32_t bitstream_buffer_id, Since we can't have more inputs than buffer indices, could we move the InputRecord data into InputBufferInfo directly, and remove the InputRecord class entirely? https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... File chrome/gpu/arc_gpu_video_decode_accelerator.h (right): https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:21: // This class is excuted in Chromium's GPU process. It takes decoding requests s/excuted/executed/ https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:23: // real VDA on Chromium. It also returns the decoded frames back to the s/real VDA/to an implementation of the VideoDecodeAccelerator interface/ https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:39: bool BindSharedMemory(PortType port, Could we make all calls (apart from perhaps Initialize()) void and report errors instead please? https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:73: // The file handle to access the buffer. It is owned by this ArcGVDA and s/ArcGVDA/class/ https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:74: // should be freed if not used. s/freed if not used/closed after use/ https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:99: // Indicates a pending EOS output buffer. When it is true, an EOS output s/Indicates a pending EOS output buffer. When it is true,/When true,/ https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:101: bool pending_eos_output_buffer_; Please move members to after methods. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:117: ArcVideoAccelerator::Client* arc_client_; We could perhaps use WeakPtr here... https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:122: PortInfo port_info_[PORT_COUNT]; Unless I'm missing something, we are not using memory_type from PortInfo, and num_buffers could be taken from input_buffer_info_.size(), and we could remove this struct and array? https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:129: // The index of buffers returned while Flushing, will be used for sending picture_buffer_ids for Pictures that were returned to us from VDA via PictureReady() while flushing. We keep them until NotifyFlushDone(); once it's called, we send one of the pending buffers from this queue, marked with an EOS flag, to arc_client_, and return the rest to VDA for reuse. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:137: }; Do we need DISALLOW_IMPLICIT_CONSTRUCTORS()?
https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:72: size_t* count) { On 2016/02/29 08:25:20, Pawel Osciak wrote: > We are not assigning to count, do we need it to be an in/out value? I believe > this is only used as a final count and not negotiable anymore? We get min. > number of buffers in ProvidePictureBuffers, send it via OnOutpufFormatChanged, > and then receive final count via SetBufferCount? That's because VDA doesn't returns false even on failure on vda->AssignPictureBuffers. I don't think we can guarantee to support any number of buffers? mmm.... maybe we can. since the buffer is not allocated from device. In that case, sure let's make count a input argument only. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:78: DVLOG(1) << "Must call setBufferFormat before setBufferCount"; On 2016/02/29 08:25:19, Pawel Osciak wrote: > VDA is now created in Initialize()... But ideally we could possibly remove the > need for SetBufferFormat() (please see other comments). Done. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:81: if (port == PORT_OUTPUT) { On 2016/02/29 08:25:19, Pawel Osciak wrote: > We should probably check we don't have any existing buffers already and refuse > this otherwise. I think VDA should handle this case? https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:91: if (port == PORT_INPUT) { On 2016/02/29 08:25:19, Pawel Osciak wrote: > Is it ok to allow SetBufferCount at any time for inputs? For example, if we are > still decoding, but client calls SetBufferCount for INPUT, we drop the inputs, > but the VDA still has them and may return them to us. So we could signal we are > done for buffers that were dropped. Perhaps better not to allow SetBufferCount > at all if we already have buffers? Will move this to be part of initialization. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:98: bool ArcGpuVideoDecodeAccelerator::SetBufferFormat(PortType port, On 2016/02/29 08:25:20, Pawel Osciak wrote: > IIRC, we could not change format and memory type after initial setup. If so, > perhaps we should pass formats to AVA::Initialize() and remove this method? I agree. But this will involve changes of different projects in both arc and chromium side. I would prefer to do that in a separate CL. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:117: config.profile = media::H264PROFILE_MAIN; On 2016/02/29 08:25:20, Pawel Osciak wrote: > On second thought, could we pass the exact profile and use it here? A VDA may > support a subset of profiles for a given codec and the VDA interface already > handles it. I don't think we know the exact profile. MediaCodecs just tell us the mime type. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:128: if (!vda_->Initialize(config, this)) { On 2016/02/29 08:25:20, Pawel Osciak wrote: > We should probably make sure vda_ is not null, otherwise the client may crash > us. Done. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:137: DVLOG(1) << "Only DMA buffer is supported for output buffers"; On 2016/02/29 08:25:19, Pawel Osciak wrote: > s/DMA buffer/dmabuf/ Done. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:172: size_t offset, On 2016/02/29 08:25:20, Pawel Osciak wrote: > off_t ? Done. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:182: if (port != PORT_INPUT) { On 2016/02/29 08:25:20, Pawel Osciak wrote: > Should we instead check against port_info->memory_type for given port? port_info is just removed. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:202: if (port != PORT_OUTPUT) { On 2016/02/29 08:25:20, Pawel Osciak wrote: > Here as well, should we instead check against port_info->memory_type for given > port? same as above. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:231: if (metadata.bytes_used == 0) { On 2016/02/29 08:25:19, Pawel Osciak wrote: > Could we handle this case in VDAs? We already do I believe. Done. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:235: if (metadata.bytes_used > input_info->length) { On 2016/02/29 08:25:19, Pawel Osciak wrote: > I think this also should be validated in the VDA, as it has to do it anyway? Yes, you're right. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:293: // TODO(owenlin): How to get visible size? On 2016/02/29 08:25:20, Pawel Osciak wrote: > Do we need it at this time for the client? Maybe not, I think client needs the visible size before getting the first decoded frame. However, I would prefer to address this issue later. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:303: // no-op On 2016/02/29 08:25:20, Pawel Osciak wrote: > I'm wondering if we should still clean up here, i.e. close the fd, mark the > buffer as dismissed and return it to the client. Next time we get it from the > client we could just ignore it. There should not be any fd owned by us and need to be closed here. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:310: if (!ValidatePortAndIndex(PORT_OUTPUT, picture.picture_buffer_id())) { On 2016/02/29 08:25:20, Pawel Osciak wrote: > Do we need to validate PR(), since it's coming from the VDA? Done. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:397: void ArcGpuVideoDecodeAccelerator::SetInputRecord(int32_t bitstream_buffer_id, On 2016/02/29 08:25:20, Pawel Osciak wrote: > Since we can't have more inputs than buffer indices, could we move the > InputRecord data into InputBufferInfo directly, and remove the InputRecord class > entirely? InputRecord could be more than InputBuffers. Assume there are only 2 input buffers, in the case of h264, the frames will be reordered so will not returned so quickly. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... File chrome/gpu/arc_gpu_video_decode_accelerator.h (right): https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:21: // This class is excuted in Chromium's GPU process. It takes decoding requests On 2016/02/29 08:25:20, Pawel Osciak wrote: > s/excuted/executed/ Done. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:23: // real VDA on Chromium. It also returns the decoded frames back to the On 2016/02/29 08:25:21, Pawel Osciak wrote: > s/real VDA/to an implementation of the VideoDecodeAccelerator interface/ Done. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:39: bool BindSharedMemory(PortType port, On 2016/02/29 08:25:20, Pawel Osciak wrote: > Could we make all calls (apart from perhaps Initialize()) void and report errors > instead please? The reason is? Let's make BindSharedMemory void, but due to the current logic in ArcCodec, it is not possible to let SetBufferCount returns void. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:73: // The file handle to access the buffer. It is owned by this ArcGVDA and On 2016/02/29 08:25:21, Pawel Osciak wrote: > s/ArcGVDA/class/ Done. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:74: // should be freed if not used. On 2016/02/29 08:25:20, Pawel Osciak wrote: > s/freed if not used/closed after use/ Done. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:99: // Indicates a pending EOS output buffer. When it is true, an EOS output On 2016/02/29 08:25:20, Pawel Osciak wrote: > s/Indicates a pending EOS output buffer. When it is true,/When true,/ Done. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:101: bool pending_eos_output_buffer_; On 2016/02/29 08:25:20, Pawel Osciak wrote: > Please move members to after methods. Done. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:117: ArcVideoAccelerator::Client* arc_client_; On 2016/02/29 08:25:20, Pawel Osciak wrote: > We could perhaps use WeakPtr here... Not so sure, WeakPtr somehow means we expect it to be vanished, but it should not. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:122: PortInfo port_info_[PORT_COUNT]; On 2016/02/29 08:25:21, Pawel Osciak wrote: > Unless I'm missing something, we are not using memory_type from PortInfo, and > num_buffers could be taken from input_buffer_info_.size(), and we could remove > this struct and array? Done. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:129: // The index of buffers returned while Flushing, will be used for sending On 2016/02/29 08:25:20, Pawel Osciak wrote: > picture_buffer_ids for Pictures that were returned to us from VDA via > PictureReady() while flushing. We keep them until NotifyFlushDone(); once it's > called, we send one of the pending buffers from this queue, marked with an EOS > flag, to arc_client_, and return the rest to VDA for reuse. Done. https://codereview.chromium.org/1549473002/diff/200001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:137: }; On 2016/02/29 08:25:21, Pawel Osciak wrote: > Do we need DISALLOW_IMPLICIT_CONSTRUCTORS()? Done.
https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_video_a... File chrome/gpu/arc_video_accelerator.h (right): https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_video_a... chrome/gpu/arc_video_accelerator.h:46: uint32_t image_size; buffer_size ?
https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:169: << ", timestamp=" << metadata.timestamp << "))"; extra )
https://codereview.chromium.org/1549473002/diff/120001/content/common/gpu/med... File content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1549473002/diff/120001/content/common/gpu/med... content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc:46: for (int32_t id = 0, n = *count; id < n; ++id) { On 2016/02/02 09:08:06, Owen Lin wrote: > On 2016/01/20 09:11:05, kcwu wrote: > > how about just > > for (int32_t id = 0; id < *count; ++id) ? > > I think compiler can do the right optimization. > > Also prevent comparing signed and unsigned numbers. If so, how about just cast explicitly? https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:32: handle = std::move(a.handle); ArcGpuVideoDecodeAccelerator::InputBufferInfo::InputBufferInfo( InputBufferInfo&& a) = default; https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:49: static const size_t MAX_BUFFER_COUNT = 128; why 128? Is the same reason as kMaxNumberOfInputRecords? https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:64: vda_.reset(content::CreateVDA(AsWeakPtr(), io_task_runner_)); base::ThreadTaskRunnerHandle::Get(). Since you expect the same thread according to your thread_checker_ checks. https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:102: if (number >= MAX_BUFFER_COUNT) { > instead of >= ? https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:134: if (port != PORT_INPUT) { How about move this check to line 130 (before ValidatePortAndIndex)? https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:156: if (port != PORT_OUTPUT) { How about move this check to line 152 (before ValidatePortAndIndex)? https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:181: vda_->Flush(); I'm not sure. Should this move to line 196 or return here? Decode() after Flush() looks weird to me. https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:184: // Mask against 30 bits, to avoid (undefined wraparound on signed integer) s/(undefined wraparound on signed integer)/(undefined) wraparound on signed integer./ https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:246: // no-op why can it be no-op? https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:255: if (picture.bitstream_buffer_id() == -1) { where is "-1" coming from? I don't find it in this file and neither mentioned in ava.h and vda.h https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... File chrome/gpu/arc_gpu_video_decode_accelerator.h (right): https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:121: // not empty), marked with an EOS flag, to arc_client_, and return the rest |arc_client_| https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:123: std::queue<uint32_t> buffers_pending_eos_; bitstream buffer id is int32_t
https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:49: static const size_t MAX_BUFFER_COUNT = 128; On 2016/03/09 07:58:21, kcwu wrote: > why 128? > Is the same reason as kMaxNumberOfInputRecords? and s/MAX_BUFFER_COUNT/kMaxBufferCount/ https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_video_a... File chrome/gpu/arc_video_accelerator.h (right): https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_video_a... chrome/gpu/arc_video_accelerator.h:36: struct BufferMetadata { How about to use "Non-Static Class Member Initializers" in this and other structs? https://chromium-cpp.appspot.com/ struct BufferMetadata { int64_t timestamp = 0; uint32_t flags = 0; uint32_t bytes_used = 0; };
https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:32: handle = std::move(a.handle); On 2016/03/09 07:58:21, kcwu wrote: > ArcGpuVideoDecodeAccelerator::InputBufferInfo::InputBufferInfo( > InputBufferInfo&& a) = default; Removed. https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:49: static const size_t MAX_BUFFER_COUNT = 128; On 2016/03/09 07:58:21, kcwu wrote: > why 128? > Is the same reason as kMaxNumberOfInputRecords? Not the exactly same purpose but it is also arbitrary chosen. Comments are added. https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:49: static const size_t MAX_BUFFER_COUNT = 128; On 2016/03/10 07:27:26, kcwu wrote: > On 2016/03/09 07:58:21, kcwu wrote: > > why 128? > > Is the same reason as kMaxNumberOfInputRecords? > > and s/MAX_BUFFER_COUNT/kMaxBufferCount/ Done. https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:64: vda_.reset(content::CreateVDA(AsWeakPtr(), io_task_runner_)); On 2016/03/09 07:58:21, kcwu wrote: > base::ThreadTaskRunnerHandle::Get(). > Since you expect the same thread according to your thread_checker_ checks. io_task_runner_ is no longer required. Removed. https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:102: if (number >= MAX_BUFFER_COUNT) { On 2016/03/09 07:58:21, kcwu wrote: > > instead of >= ? Done. https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:134: if (port != PORT_INPUT) { On 2016/03/09 07:58:21, kcwu wrote: > How about move this check to line 130 (before ValidatePortAndIndex)? Done. https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:156: if (port != PORT_OUTPUT) { On 2016/03/09 07:58:21, kcwu wrote: > How about move this check to line 152 (before ValidatePortAndIndex)? Done. https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:169: << ", timestamp=" << metadata.timestamp << "))"; On 2016/03/07 14:04:33, kcwu wrote: > extra ) Done. https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:181: vda_->Flush(); On 2016/03/09 07:58:21, kcwu wrote: > I'm not sure. Should this move to line 196 or return here? Decode() after > Flush() looks weird to me. Move to line 196. We need to send the content to be decoded first and then call flush. https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:184: // Mask against 30 bits, to avoid (undefined wraparound on signed integer) On 2016/03/09 07:58:21, kcwu wrote: > s/(undefined wraparound on signed integer)/(undefined) wraparound on signed > integer./ Done. https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:246: // no-op On 2016/03/09 07:58:21, kcwu wrote: > why can it be no-op? VDA has its owned FDs of the picture buffers and will close those FD after use. The picture buffer on ARC is managed by the native window. We don't need to do anything here. https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:255: if (picture.bitstream_buffer_id() == -1) { On 2016/03/09 07:58:21, kcwu wrote: > where is "-1" coming from? > I don't find it in this file and neither mentioned in ava.h and vda.h It would be defined in the new Flush(bool return_buffers); function. https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... File chrome/gpu/arc_gpu_video_decode_accelerator.h (right): https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:121: // not empty), marked with an EOS flag, to arc_client_, and return the rest On 2016/03/09 07:58:21, kcwu wrote: > |arc_client_| Done. https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:123: std::queue<uint32_t> buffers_pending_eos_; On 2016/03/09 07:58:21, kcwu wrote: > bitstream buffer id is int32_t Done. https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_video_a... File chrome/gpu/arc_video_accelerator.h (right): https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_video_a... chrome/gpu/arc_video_accelerator.h:36: struct BufferMetadata { On 2016/03/10 07:27:26, kcwu wrote: > How about to use "Non-Static Class Member Initializers" in this and other > structs? > https://chromium-cpp.appspot.com/ > > struct BufferMetadata { > int64_t timestamp = 0; > uint32_t flags = 0; > uint32_t bytes_used = 0; > }; Nice. https://codereview.chromium.org/1549473002/diff/220001/chrome/gpu/arc_video_a... chrome/gpu/arc_video_accelerator.h:46: uint32_t image_size; On 2016/03/04 13:32:59, kcwu wrote: > buffer_size ? Done.
lgtm https://codereview.chromium.org/1549473002/diff/240001/chrome/gpu/arc_gpu_vid... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1549473002/diff/240001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:10: // TODO use Pawel's factory instead Remove this TODO? https://codereview.chromium.org/1549473002/diff/240001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:306: extra blank line
owenlin@chromium.org changed reviewers: + jochen@chromium.org
Hi jochen@, please help us do a owner's review. Thanks.
jochen@chromium.org changed reviewers: + kbr@chromium.org
+kbr because gpu
LGTM as far as I understand it. Please add an OWNERS file in the new chrome/gpu directory. Perhaps someone from the media team who understands this code should be on it. I suggest also piman and sievers as they are OWNERS of src/gpu/ .
owenlin@google.com changed reviewers: + owenlin@google.com
PTAL. Also add posciak@ in the OWNERS file. He is the OWNER of most VideoDecodeAccelerator implementations (i.e., content/common/gpu/media). https://codereview.chromium.org/1549473002/diff/240001/chrome/gpu/arc_gpu_vid... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1549473002/diff/240001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:10: // TODO use Pawel's factory instead On 2016/03/14 09:37:53, kcwu wrote: > Remove this TODO? Done. https://codereview.chromium.org/1549473002/diff/240001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:306: On 2016/03/14 09:37:53, kcwu wrote: > extra blank line Done.
owenlin@google.com changed reviewers: - owenlin@google.com
https://codereview.chromium.org/1549473002/diff/280001/chrome/gpu/arc_gpu_vid... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1549473002/diff/280001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:11: #undef DVLOG remember to remove before commit
need to rebase after pawel's GpuVideoDecodeAcceleratorFactory change.
Hi jochen@, we still need a owner's lgtm for this CL. PTAL. Thanks.
I'm thinking we need to handle unexpected calls from untrusted client. https://codereview.chromium.org/1549473002/diff/280001/chrome/gpu/arc_gpu_vid... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1549473002/diff/280001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:81: void ArcGpuVideoDecodeAccelerator::SetNumberOfOutputBuffers(size_t number) { what if SetNumberOfOutputBuffers called during decoding? https://codereview.chromium.org/1549473002/diff/280001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:96: vda_->AssignPictureBuffers(buffers); I'm wondering if AssignPictureBuffers() is called at unexpected time (say, before ProvidePictureBuffers() happens). Should this class check that, or each VDA will handle such case? https://codereview.chromium.org/1549473002/diff/280001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:147: void ArcGpuVideoDecodeAccelerator::UseBuffer(PortType port, what if UseBuffer called with index which is already send to decode queue? https://codereview.chromium.org/1549473002/diff/280001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:172: vda_->Decode(media::BitstreamBuffer( We need to check |vda_| is valid for all methods to make sure Initialize()ed.
https://codereview.chromium.org/1549473002/diff/280001/chrome/gpu/arc_gpu_vid... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1549473002/diff/280001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:11: #undef DVLOG On 2016/03/16 14:20:48, kcwu wrote: > remember to remove before commit Done. https://codereview.chromium.org/1549473002/diff/280001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:81: void ArcGpuVideoDecodeAccelerator::SetNumberOfOutputBuffers(size_t number) { On 2016/03/17 13:10:47, kcwu wrote: > what if SetNumberOfOutputBuffers called during decoding? Each VDA should handle the case. https://codereview.chromium.org/1549473002/diff/280001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:96: vda_->AssignPictureBuffers(buffers); On 2016/03/17 13:10:47, kcwu wrote: > I'm wondering if AssignPictureBuffers() is called at unexpected time (say, > before ProvidePictureBuffers() happens). > Should this class check that, or each VDA will handle such case? VDA should handle the error and call NotifyError(ILLEGAL_STATE). https://codereview.chromium.org/1549473002/diff/280001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:147: void ArcGpuVideoDecodeAccelerator::UseBuffer(PortType port, On 2016/03/17 13:10:47, kcwu wrote: > what if UseBuffer called with index which is already send to decode queue? VDA should handle this. https://codereview.chromium.org/1549473002/diff/280001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:172: vda_->Decode(media::BitstreamBuffer( On 2016/03/17 13:10:47, kcwu wrote: > We need to check |vda_| is valid for all methods to make sure Initialize()ed. Yes, we need. Code modified.
https://codereview.chromium.org/1549473002/diff/300001/chrome/gpu/arc_gpu_vid... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1549473002/diff/300001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:83: arc_client_->OnError(ILLEGAL_STATE); If not Initialize() yet, arc_client_ will be probably null as well.
wuchengli@chromium.org changed reviewers: + wuchengli@chromium.org
jochen@ Can you LGTM now that Ken approved? Ken is not in OWNERS file.
where is this code used? it seems like the new classes get nowhere instantiated? where are the tests?
On 2016/03/18 15:33:46, jochen - slow wrote: > where is this code used? it seems like the new classes get nowhere instantiated? > > where are the tests? The code will be integrated with other CLs under review: https://codereview.chromium.org/1641353003/ (This will instantiated us.) https://codereview.chromium.org/1745903002/ About the unittest, there is not much logic in the code. Just like GpuVideoDecodeAccelerator (which has no unittest either), the main task of ArcGpuVideoDecodeAccelerator is taking video decoding requests from ARC to a real VideoDecodeAccelerator(VDA) and also sending back the decoded frames.
https://codereview.chromium.org/1549473002/diff/300001/chrome/gpu/arc_video_a... File chrome/gpu/arc_video_accelerator.h (right): https://codereview.chromium.org/1549473002/diff/300001/chrome/gpu/arc_video_a... chrome/gpu/arc_video_accelerator.h:23: BUFFER_FLAG_EOS = 1, Perhaps 1 << 0 to further emphasize these to be bitfield values (e.g. like https://code.google.com/p/chromium/codesearch#chromium/src/base/files/file.h&...) https://codereview.chromium.org/1549473002/diff/300001/chrome/gpu/arc_video_a... chrome/gpu/arc_video_accelerator.h:28: uint32_t flags = 0; Please add a comment this should use values from BufferFlag. https://codereview.chromium.org/1549473002/diff/300001/chrome/gpu/arc_video_a... chrome/gpu/arc_video_accelerator.h:36: // minimal number of buffers required to process the video. s/minimal/Minimum/ s/process the video/decode\/encode the stream/ https://codereview.chromium.org/1549473002/diff/300001/chrome/gpu/arc_video_a... chrome/gpu/arc_video_accelerator.h:49: // ArcCodec implements ArcVideoAccelerator::Client and is responsible for Please remove or rephrase this sentence so that it applies to ArcVideoAccelerator. https://codereview.chromium.org/1549473002/diff/300001/chrome/gpu/arc_video_a... chrome/gpu/arc_video_accelerator.h:69: // TODO: Add output_pixel_format. For now only the native pixel format TODO(owner): https://codereview.chromium.org/1549473002/diff/300001/chrome/gpu/arc_video_a... chrome/gpu/arc_video_accelerator.h:73: // The callbacks of the ArcVideoAccelerator. ArcCodec implmenets this Please update the last sentence. https://codereview.chromium.org/1549473002/diff/300001/chrome/gpu/arc_video_a... chrome/gpu/arc_video_accelerator.h:80: // only when the accelerator processes the input buffer and tried to Is this still applicable? I think we may OnError() in other situations? https://codereview.chromium.org/1549473002/diff/300001/chrome/gpu/arc_video_a... chrome/gpu/arc_video_accelerator.h:86: // it can be filled with new content. For output buffer, it is ready to s/buffer, it can be filled/buffers, the Client may fill them/ s/buffer, it is/buffers indicates they are/ https://codereview.chromium.org/1549473002/diff/300001/chrome/gpu/arc_video_a... chrome/gpu/arc_video_accelerator.h:97: virtual bool Initialize(const Config& config, Client* client) = 0; Please document. https://codereview.chromium.org/1549473002/diff/300001/chrome/gpu/arc_video_a... chrome/gpu/arc_video_accelerator.h:100: // port and index. A buffer must be bound before asking the accelerator to A buffer must be successfully bound using this method before it can be passed to the accelerator via UseBuffer(). Already bound buffers may be reused multiple times without additional calls to BindSharedMemory(). https://codereview.chromium.org/1549473002/diff/300001/chrome/gpu/arc_video_a... chrome/gpu/arc_video_accelerator.h:108: // Assigns a graphic buffer to be used for the accelerator at the specified s/graphic// https://codereview.chromium.org/1549473002/diff/300001/chrome/gpu/arc_video_a... chrome/gpu/arc_video_accelerator.h:120: // Sets the number of output buffers. Please document whether this can fail and what happens then.
https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:31: // buffers used is requested from the client side (less trusted). s/less trusted/untrusted/ https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:32: static const size_t kMaxBufferCount = 128; Perhaps anonymous namespace? https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:51: input_buffer_info_.resize(config.num_input_buffers); We should probably clear() first to release any previous buffers (or just always deny re-Initialize()). https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:93: for (int32_t id = 0, n = number; id < n; ++id) { Is n needed? https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:127: InputBufferInfo* input_info = &input_buffer_info_[index]; Do we need to make sure we don't bind over existing buffer info and lose the ref while the buffer is being processed if BindSharedMemory() is called twice before we return it? https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:146: DLOG(ERROR) << "GraphicBuffer is only supported for input"; s/GraphicBuffer/Dmabuf/ https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:184: return; Should we first dup, and then create the InputRecord to avoid a stale record if this fails? https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:221: // TODO(owenlin): use VDA::GetOutputFormat() here and calculate correct Could we use it already, potentially only handling YUV and just denying other formats for now? https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:331: DCHECK(!pending_import_buffer_[index].is_valid()); This is probably not needed since we call release() explicitly. https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:344: const size_t kMaxNumberOfInputRecords = 128; Should we use kMaxBufferCount? https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:345: if (input_records_.size() > kMaxNumberOfInputRecords) Please add a comment why we keep a rolling record of buffers and only pop_back() when we reach the limit. https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... File chrome/gpu/arc_gpu_video_decode_accelerator.h (right): https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:21: // This class is executed in Chromium's GPU process. It takes decoding requests s/Chromium's// https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:22: // from ARC via IPC channels and translates and send those requests to an s/send/sends/ https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:23: // implementation of the VideoDecodeAccelerator interface on Chromium. It also s/the VideoDecodeAccelerator interface on Chromium/media::VideoDecodeAccelerator/ https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:60: struct InputRecord { Please document the difference between InputRecord and InputBufferInfo. https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:85: // Sets InputRecord for the given |bitstream_buffer_id|. The |buffer_index| s/Sets/Creates an/ https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:88: void SetInputRecord(int32_t bitstream_buffer_id, s/Set/Create/ https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:99: scoped_ptr<media::VideoDecodeAccelerator> vda_; Please add an empty line above. https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:106: base::Closure reset_done_callback_; Unused? https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:113: // The |picture_buffer_id|s for Picutres that were returned to us from VDA s/Picutres/Pictures/ https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:119: std::list<InputRecord> input_records_; Please add an empty line above and documentation. https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:123: // be used yet. Will call VDA::ImportBufferForPicture() when those buffer are s/but not be used yet/but cannot be used before the Client calls useBuffer() on them/ s/buffer/buffers/ https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:125: std::vector<base::ScopedFD> pending_import_buffer_; s/pending_import_buffer_/buffers_pending_import_/ https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:126: scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_; Unused?
https://codereview.chromium.org/1549473002/diff/300001/chrome/gpu/arc_gpu_vid... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1549473002/diff/300001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:83: arc_client_->OnError(ILLEGAL_STATE); On 2016/03/18 06:30:11, kcwu wrote: > If not Initialize() yet, arc_client_ will be probably null as well. Done. https://codereview.chromium.org/1549473002/diff/300001/chrome/gpu/arc_video_a... File chrome/gpu/arc_video_accelerator.h (right): https://codereview.chromium.org/1549473002/diff/300001/chrome/gpu/arc_video_a... chrome/gpu/arc_video_accelerator.h:23: BUFFER_FLAG_EOS = 1, On 2016/03/22 08:36:11, Pawel Osciak wrote: > Perhaps 1 << 0 to further emphasize these to be bitfield values (e.g. like > https://code.google.com/p/chromium/codesearch#chromium/src/base/files/file.h&...) Done. https://codereview.chromium.org/1549473002/diff/300001/chrome/gpu/arc_video_a... chrome/gpu/arc_video_accelerator.h:28: uint32_t flags = 0; On 2016/03/22 08:36:11, Pawel Osciak wrote: > Please add a comment this should use values from BufferFlag. Done. https://codereview.chromium.org/1549473002/diff/300001/chrome/gpu/arc_video_a... chrome/gpu/arc_video_accelerator.h:36: // minimal number of buffers required to process the video. On 2016/03/22 08:36:11, Pawel Osciak wrote: > s/minimal/Minimum/ > s/process the video/decode\/encode the stream/ Done. https://codereview.chromium.org/1549473002/diff/300001/chrome/gpu/arc_video_a... chrome/gpu/arc_video_accelerator.h:49: // ArcCodec implements ArcVideoAccelerator::Client and is responsible for On 2016/03/22 08:36:11, Pawel Osciak wrote: > Please remove or rephrase this sentence so that it applies to > ArcVideoAccelerator. Done. https://codereview.chromium.org/1549473002/diff/300001/chrome/gpu/arc_video_a... chrome/gpu/arc_video_accelerator.h:69: // TODO: Add output_pixel_format. For now only the native pixel format On 2016/03/22 08:36:11, Pawel Osciak wrote: > TODO(owner): Done. https://codereview.chromium.org/1549473002/diff/300001/chrome/gpu/arc_video_a... chrome/gpu/arc_video_accelerator.h:73: // The callbacks of the ArcVideoAccelerator. ArcCodec implmenets this On 2016/03/22 08:36:11, Pawel Osciak wrote: > Please update the last sentence. Done. https://codereview.chromium.org/1549473002/diff/300001/chrome/gpu/arc_video_a... chrome/gpu/arc_video_accelerator.h:80: // only when the accelerator processes the input buffer and tried to On 2016/03/22 08:36:10, Pawel Osciak wrote: > Is this still applicable? I think we may OnError() in other situations? Indeed. Remove the last sentence. https://codereview.chromium.org/1549473002/diff/300001/chrome/gpu/arc_video_a... chrome/gpu/arc_video_accelerator.h:86: // it can be filled with new content. For output buffer, it is ready to On 2016/03/22 08:36:11, Pawel Osciak wrote: > s/buffer, it can be filled/buffers, the Client may fill them/ > s/buffer, it is/buffers indicates they are/ Done. https://codereview.chromium.org/1549473002/diff/300001/chrome/gpu/arc_video_a... chrome/gpu/arc_video_accelerator.h:97: virtual bool Initialize(const Config& config, Client* client) = 0; On 2016/03/22 08:36:11, Pawel Osciak wrote: > Please document. Done. https://codereview.chromium.org/1549473002/diff/300001/chrome/gpu/arc_video_a... chrome/gpu/arc_video_accelerator.h:100: // port and index. A buffer must be bound before asking the accelerator to On 2016/03/22 08:36:10, Pawel Osciak wrote: > A buffer must be successfully bound using this method before it can be passed to > the accelerator via UseBuffer(). Already bound buffers may be reused multiple > times without additional calls to BindSharedMemory(). Done. https://codereview.chromium.org/1549473002/diff/300001/chrome/gpu/arc_video_a... chrome/gpu/arc_video_accelerator.h:108: // Assigns a graphic buffer to be used for the accelerator at the specified On 2016/03/22 08:36:11, Pawel Osciak wrote: > s/graphic// Done. https://codereview.chromium.org/1549473002/diff/300001/chrome/gpu/arc_video_a... chrome/gpu/arc_video_accelerator.h:120: // Sets the number of output buffers. On 2016/03/22 08:36:10, Pawel Osciak wrote: > Please document whether this can fail and what happens then. This is not likely to fail in current implementation. Kung-Che did suggest add a limits to prevent malicious clients. https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:31: // buffers used is requested from the client side (less trusted). On 2016/03/22 10:21:12, Pawel Osciak wrote: > s/less trusted/untrusted/ Done. https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:32: static const size_t kMaxBufferCount = 128; On 2016/03/22 10:21:12, Pawel Osciak wrote: > Perhaps anonymous namespace? Done. https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:51: input_buffer_info_.resize(config.num_input_buffers); On 2016/03/22 10:21:12, Pawel Osciak wrote: > We should probably clear() first to release any previous buffers (or just always > deny re-Initialize()). For now, let's deny re-Initialize(). https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:93: for (int32_t id = 0, n = number; id < n; ++id) { On 2016/03/22 10:21:12, Pawel Osciak wrote: > Is n needed? Just to prevent warning about comparing signed and unsigned integers. https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:127: InputBufferInfo* input_info = &input_buffer_info_[index]; On 2016/03/22 10:21:12, Pawel Osciak wrote: > Do we need to make sure we don't bind over existing buffer info and lose the ref > while the buffer is being processed if BindSharedMemory() is called twice before > we return it? Actually, it is fine to bind another buffer at any time. We dup() the fd before we send it to the accelerator. https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:146: DLOG(ERROR) << "GraphicBuffer is only supported for input"; On 2016/03/22 10:21:12, Pawel Osciak wrote: > s/GraphicBuffer/Dmabuf/ Done. https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:184: return; On 2016/03/22 10:21:12, Pawel Osciak wrote: > Should we first dup, and then create the InputRecord to avoid a stale record if > this fails? Done. https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:221: // TODO(owenlin): use VDA::GetOutputFormat() here and calculate correct On 2016/03/22 10:21:12, Pawel Osciak wrote: > Could we use it already, potentially only handling YUV and just denying other > formats for now? It is still not clear how should this info get handled on Android side. Can we consider this later. https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:331: DCHECK(!pending_import_buffer_[index].is_valid()); On 2016/03/22 10:21:12, Pawel Osciak wrote: > This is probably not needed since we call release() explicitly. Done. https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:344: const size_t kMaxNumberOfInputRecords = 128; On 2016/03/22 10:21:12, Pawel Osciak wrote: > Should we use kMaxBufferCount? They are different in meaning. The MaxNumberOfInputRecords is to ensure the record is kept before we returned the corresponding output buffers. The kMaxBufferCount just a arbitrary chosen value to prevent client request too much buffers. https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:345: if (input_records_.size() > kMaxNumberOfInputRecords) On 2016/03/22 10:21:12, Pawel Osciak wrote: > Please add a comment why we keep a rolling record of buffers and only pop_back() > when we reach the limit. Done. https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... File chrome/gpu/arc_gpu_video_decode_accelerator.h (right): https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:21: // This class is executed in Chromium's GPU process. It takes decoding requests On 2016/03/22 10:21:13, Pawel Osciak wrote: > s/Chromium's// Done. https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:22: // from ARC via IPC channels and translates and send those requests to an On 2016/03/22 10:21:12, Pawel Osciak wrote: > s/send/sends/ Done. https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:23: // implementation of the VideoDecodeAccelerator interface on Chromium. It also On 2016/03/22 10:21:12, Pawel Osciak wrote: > s/the VideoDecodeAccelerator interface on > Chromium/media::VideoDecodeAccelerator/ Done. https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:60: struct InputRecord { On 2016/03/22 10:21:12, Pawel Osciak wrote: > Please document the difference between InputRecord and InputBufferInfo. Done. https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:85: // Sets InputRecord for the given |bitstream_buffer_id|. The |buffer_index| On 2016/03/22 10:21:13, Pawel Osciak wrote: > s/Sets/Creates an/ Done. https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:88: void SetInputRecord(int32_t bitstream_buffer_id, On 2016/03/22 10:21:13, Pawel Osciak wrote: > s/Set/Create/ Done. https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:99: scoped_ptr<media::VideoDecodeAccelerator> vda_; On 2016/03/22 10:21:13, Pawel Osciak wrote: > Please add an empty line above. Done. https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:106: base::Closure reset_done_callback_; On 2016/03/22 10:21:12, Pawel Osciak wrote: > Unused? Done. https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:113: // The |picture_buffer_id|s for Picutres that were returned to us from VDA On 2016/03/22 10:21:12, Pawel Osciak wrote: > s/Picutres/Pictures/ Done. https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:119: std::list<InputRecord> input_records_; On 2016/03/22 10:21:13, Pawel Osciak wrote: > Please add an empty line above and documentation. Done. https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:123: // be used yet. Will call VDA::ImportBufferForPicture() when those buffer are On 2016/03/22 10:21:13, Pawel Osciak wrote: > s/but not be used yet/but cannot be used before the Client calls useBuffer() on > them/ > s/buffer/buffers/ Those buffer *can* be used whenever Client calls useBuffer. They were kept for useBuffer(). https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:125: std::vector<base::ScopedFD> pending_import_buffer_; On 2016/03/22 10:21:13, Pawel Osciak wrote: > s/pending_import_buffer_/buffers_pending_import_/ Done. https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.h:126: scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_; On 2016/03/22 10:21:13, Pawel Osciak wrote: > Unused? Done.
jochen@chromium.org changed reviewers: + cpu@chromium.org, jam@chromium.org
+cpu/jam who review the CL where this class is getting used please try not to spread out a feature to disjoint subsets of owners
On 2016/03/24 14:48:53, jochen - slow wrote: > +cpu/jam who review the CL where this class is getting used > > please try not to spread out a feature to disjoint subsets of owners rubberstamp lgtm for adding new directory. i defer to other reviewers on the correctness of this video decoding code.
agvda.* lgtm % nits https://chromiumcodereview.appspot.com/1549473002/diff/340001/chrome/gpu/arc_... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/1549473002/diff/340001/chrome/gpu/arc_... chrome/gpu/arc_gpu_video_decode_accelerator.cc:127: InputBufferInfo* input_info = &input_buffer_info_[index]; On 2016/03/24 03:01:11, Owen Lin wrote: > On 2016/03/22 10:21:12, Pawel Osciak wrote: > > Do we need to make sure we don't bind over existing buffer info and lose the > ref > > while the buffer is being processed if BindSharedMemory() is called twice > before > > we return it? > > Actually, it is fine to bind another buffer at any time. We dup() the fd before > we send it to the accelerator. > Acknowledged. https://chromiumcodereview.appspot.com/1549473002/diff/340001/chrome/gpu/arc_... chrome/gpu/arc_gpu_video_decode_accelerator.cc:221: // TODO(owenlin): use VDA::GetOutputFormat() here and calculate correct On 2016/03/24 03:01:11, Owen Lin wrote: > On 2016/03/22 10:21:12, Pawel Osciak wrote: > > Could we use it already, potentially only handling YUV and just denying other > > formats for now? > > It is still not clear how should this info get handled on Android side. Can we > consider this later. I basically meant this (possibly replacing PIXEL_FORMAT_NV12 with the format (formats) we are currently supporting): if (vda_->GetOutputFormat() != media::PIXEL_FORMAT_NV12) { // failure } VideoFormat video_format; // same as below... https://chromiumcodereview.appspot.com/1549473002/diff/340001/chrome/gpu/arc_... chrome/gpu/arc_gpu_video_decode_accelerator.cc:344: const size_t kMaxNumberOfInputRecords = 128; On 2016/03/24 03:01:11, Owen Lin wrote: > On 2016/03/22 10:21:12, Pawel Osciak wrote: > > Should we use kMaxBufferCount? > > They are different in meaning. The MaxNumberOfInputRecords is to ensure the > record is kept before we returned the corresponding output buffers. > > The kMaxBufferCount just a arbitrary chosen value to prevent client request too > much buffers. Acknowledged. https://chromiumcodereview.appspot.com/1549473002/diff/360001/chrome/gpu/arc_... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/1549473002/diff/360001/chrome/gpu/arc_... chrome/gpu/arc_gpu_video_decode_accelerator.cc:102: for (int32_t id = 0; static_cast<size_t>(id) < number; ++id) { Perhaps we could instead try: for (size_t i = 0; i < number; ++i) { ...PictureBuffer(i, ...); ? } maybe with cast PictureBuffer(checked_cast<>(i),...) https://chromiumcodereview.appspot.com/1549473002/diff/360001/chrome/gpu/arc_... chrome/gpu/arc_gpu_video_decode_accelerator.cc:155: DLOG(ERROR) << "DmaBuffer is only supported for input"; s/DmaBuffer/Dmabuf/ https://chromiumcodereview.appspot.com/1549473002/diff/360001/chrome/gpu/arc_... chrome/gpu/arc_gpu_video_decode_accelerator.cc:356: // others. Actually, it's needed in two situations, when input is returned, and when the corresponding output is returned? https://chromiumcodereview.appspot.com/1549473002/diff/360001/chrome/gpu/arc_... File chrome/gpu/arc_video_accelerator.h (right): https://chromiumcodereview.appspot.com/1549473002/diff/360001/chrome/gpu/arc_... chrome/gpu/arc_video_accelerator.h:36: // Minimal number of buffers required to decode/encode the stream. s/Minimal/Minimum/ https://chromiumcodereview.appspot.com/1549473002/diff/360001/chrome/gpu/arc_... chrome/gpu/arc_video_accelerator.h:46: class ArcVideoAccelerator { We should still have some comment what this class is... https://chromiumcodereview.appspot.com/1549473002/diff/360001/chrome/gpu/arc_... chrome/gpu/arc_video_accelerator.h:128: // be called. Afterwords, all buffers won't be accessed by the accelerator s/Afterwords/Afterwards/
https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1549473002/diff/340001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:221: // TODO(owenlin): use VDA::GetOutputFormat() here and calculate correct On 2016/03/25 08:38:44, Pawel Osciak wrote: > On 2016/03/24 03:01:11, Owen Lin wrote: > > On 2016/03/22 10:21:12, Pawel Osciak wrote: > > > Could we use it already, potentially only handling YUV and just denying > other > > > formats for now? > > > > It is still not clear how should this info get handled on Android side. Can we > > consider this later. > > I basically meant this (possibly replacing PIXEL_FORMAT_NV12 with the format > (formats) we are currently supporting): > > if (vda_->GetOutputFormat() != media::PIXEL_FORMAT_NV12) { > // failure > } > > VideoFormat video_format; > // same as below... Done. https://codereview.chromium.org/1549473002/diff/360001/chrome/gpu/arc_gpu_vid... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1549473002/diff/360001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:102: for (int32_t id = 0; static_cast<size_t>(id) < number; ++id) { On 2016/03/25 08:38:44, Pawel Osciak wrote: > Perhaps we could instead try: > > for (size_t i = 0; i < number; ++i) { > ...PictureBuffer(i, ...); ? > } > > maybe with cast PictureBuffer(checked_cast<>(i),...) Done. https://codereview.chromium.org/1549473002/diff/360001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:155: DLOG(ERROR) << "DmaBuffer is only supported for input"; On 2016/03/25 08:38:45, Pawel Osciak wrote: > s/DmaBuffer/Dmabuf/ Done. https://codereview.chromium.org/1549473002/diff/360001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:356: // others. On 2016/03/25 08:38:44, Pawel Osciak wrote: > Actually, it's needed in two situations, when input is returned, and when the > corresponding output is returned? Document is updated. https://codereview.chromium.org/1549473002/diff/360001/chrome/gpu/arc_video_a... File chrome/gpu/arc_video_accelerator.h (right): https://codereview.chromium.org/1549473002/diff/360001/chrome/gpu/arc_video_a... chrome/gpu/arc_video_accelerator.h:36: // Minimal number of buffers required to decode/encode the stream. On 2016/03/25 08:38:45, Pawel Osciak wrote: > s/Minimal/Minimum/ Done. https://codereview.chromium.org/1549473002/diff/360001/chrome/gpu/arc_video_a... chrome/gpu/arc_video_accelerator.h:46: class ArcVideoAccelerator { On 2016/03/25 08:38:45, Pawel Osciak wrote: > We should still have some comment what this class is... Done. https://codereview.chromium.org/1549473002/diff/360001/chrome/gpu/arc_video_a... chrome/gpu/arc_video_accelerator.h:128: // be called. Afterwords, all buffers won't be accessed by the accelerator On 2016/03/25 08:38:45, Pawel Osciak wrote: > s/Afterwords/Afterwards/ Done.
lgtm % nits https://codereview.chromium.org/1549473002/diff/380001/chrome/gpu/arc_gpu_vid... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1549473002/diff/380001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:104: // TODO: Make sure the |coded_size| is what we want. TODOs must always have an owner ("TODO(owner)") https://codereview.chromium.org/1549473002/diff/380001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:240: video_format.pixel_format = HAL_PIXEL_FORMAT_YCbCr_420_888; Perhaps we should add a comment here that HAL_PIXEL_FORMAT_YCbCr_420_888 actually handles all 420 formats, with both orderings of chroma (CbCr and CrCb) as well as planar and semiplanar layouts, so it's ok to use it for all the above? https://codereview.chromium.org/1549473002/diff/380001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:243: DLOG(ERROR) << "Format not supported: " << vda_->GetOutputFormat(); s/vda_->GetOutputFormat()/output_format/
https://chromiumcodereview.appspot.com/1549473002/diff/380001/chrome/gpu/arc_... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/1549473002/diff/380001/chrome/gpu/arc_... chrome/gpu/arc_gpu_video_decode_accelerator.cc:104: // TODO: Make sure the |coded_size| is what we want. On 2016/03/29 08:05:28, Pawel Osciak wrote: > TODOs must always have an owner ("TODO(owner)") Done. https://chromiumcodereview.appspot.com/1549473002/diff/380001/chrome/gpu/arc_... chrome/gpu/arc_gpu_video_decode_accelerator.cc:240: video_format.pixel_format = HAL_PIXEL_FORMAT_YCbCr_420_888; On 2016/03/29 08:05:28, Pawel Osciak wrote: > Perhaps we should add a comment here that HAL_PIXEL_FORMAT_YCbCr_420_888 > actually handles all 420 formats, with both orderings of chroma (CbCr and CrCb) > as well as planar and semiplanar layouts, so it's ok to use it for all the > above? Done. https://chromiumcodereview.appspot.com/1549473002/diff/380001/chrome/gpu/arc_... chrome/gpu/arc_gpu_video_decode_accelerator.cc:243: DLOG(ERROR) << "Format not supported: " << vda_->GetOutputFormat(); On 2016/03/29 08:05:28, Pawel Osciak wrote: > s/vda_->GetOutputFormat()/output_format/ Done.
Patchset #19 (id:440001) has been deleted
lgtm
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/1549473002/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1549473002/480001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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/1549473002/500001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1549473002/500001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by owenlin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kcwu@chromium.org, kbr@chromium.org, wuchengli@chromium.org, jam@chromium.org, posciak@chromium.org, cpu@chromium.org Link to the patchset: https://codereview.chromium.org/1549473002/#ps520001 (title: "add out-of-line ctor/dtor")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1549473002/520001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1549473002/520001
Message was sent while issue was closed.
Description was changed from ========== Add ArcGpuVideoDecodeAccelerator. This class runs in the GPU process. It takes video decoding requests from the ARC and talk with real ArcVDA implementations to decode the video. BUG=b/25829285 Test=Compile the code on veyron_minnie. ========== to ========== Add ArcGpuVideoDecodeAccelerator. This class runs in the GPU process. It takes video decoding requests from the ARC and talk with real ArcVDA implementations to decode the video. BUG=b/25829285 Test=Compile the code on veyron_minnie. ==========
Message was sent while issue was closed.
Committed patchset #22 (id:520001)
Message was sent while issue was closed.
Description was changed from ========== Add ArcGpuVideoDecodeAccelerator. This class runs in the GPU process. It takes video decoding requests from the ARC and talk with real ArcVDA implementations to decode the video. BUG=b/25829285 Test=Compile the code on veyron_minnie. ========== to ========== Add ArcGpuVideoDecodeAccelerator. This class runs in the GPU process. It takes video decoding requests from the ARC and talk with real ArcVDA implementations to decode the video. BUG=b/25829285 Test=Compile the code on veyron_minnie. Committed: https://crrev.com/76e5c4476a8a2cfba57c2e03216f1ce6dc7762c0 Cr-Commit-Position: refs/heads/master@{#388525} ==========
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/76e5c4476a8a2cfba57c2e03216f1ce6dc7762c0 Cr-Commit-Position: refs/heads/master@{#388525} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
