Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1953683002:
Add stride for imported Dmabuf in ArcVideoAccelerator. (Closed)
|
Created:
4 years, 7 months ago by Owen Lin Modified:
4 years, 7 months ago Reviewers:
dcheng, tfiga, rickyz (no longer on Chrome), kcwu, Pawel Osciak, scherkus (not reviewing) CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), piman+watch_chromium.org, posciak+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, wuchengli Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd stride for imported Dmabuf in ArcVideoAccelerator.
We need the stride information so that the hardware codec knows
how to fill the buffer.
BUG=b/28783710
TEST=Play video in YouTube and ApiDemos on ARC
Committed: https://crrev.com/df2a3c6dbef2976dc03827d99c0370aa479dfc5a
Cr-Commit-Position: refs/heads/master@{#395285}
Patch Set 1 #
Total comments: 9
Patch Set 2 : address kcwu's comments #
Total comments: 6
Patch Set 3 : address dcheng's review comments #
Total comments: 10
Patch Set 4 : address dcheng's comments #Patch Set 5 : address review comments #
Total comments: 1
Patch Set 6 : #Patch Set 7 : reject negative stride #Patch Set 8 : verify the stride. #
Total comments: 4
Patch Set 9 : address dcheng's comments #
Total comments: 1
Patch Set 10 : rebase and update video host's version #Patch Set 11 : address posciak's comments #
Messages
Total messages: 38 (12 generated)
owenlin@chromium.org changed reviewers: + kcwu@chromium.org, posciak@chromium.org, wuchengli@chromium.org
PTAL.
https://codereview.chromium.org/1953683002/diff/1/chrome/gpu/arc_gpu_video_de... File chrome/gpu/arc_gpu_video_decode_accelerator.h (right): https://codereview.chromium.org/1953683002/diff/1/chrome/gpu/arc_gpu_video_de... chrome/gpu/arc_gpu_video_decode_accelerator.h:85: InputBufferInfo() = default; IIUC, please put these "= default" lines in .cpp in order to avoid compiler to generate these default methods more than once. https://codereview.chromium.org/1953683002/diff/1/chrome/gpu/arc_gpu_video_de... chrome/gpu/arc_gpu_video_decode_accelerator.h:93: int32_t stride; = 0 https://codereview.chromium.org/1953683002/diff/1/chrome/gpu/arc_video_accele... File chrome/gpu/arc_video_accelerator.h (right): https://codereview.chromium.org/1953683002/diff/1/chrome/gpu/arc_video_accele... chrome/gpu/arc_video_accelerator.h:128: int32_t stride) = 0; add comments that stride is counting in bytes. https://codereview.chromium.org/1953683002/diff/1/components/arc/common/video... File components/arc/common/video_accelerator.mojom (right): https://codereview.chromium.org/1953683002/diff/1/components/arc/common/video... components/arc/common/video_accelerator.mojom:64: BindDmabuf@2(PortType port, uint32 index, handle dmabuf_fd, int32 stride); uint32
owenlin@chromium.org changed reviewers: + dcheng@chromium.org
Hi dcheng@, please help reviewing the mojom file. So many thanks. https://codereview.chromium.org/1953683002/diff/1/chrome/gpu/arc_gpu_video_de... File chrome/gpu/arc_gpu_video_decode_accelerator.h (right): https://codereview.chromium.org/1953683002/diff/1/chrome/gpu/arc_gpu_video_de... chrome/gpu/arc_gpu_video_decode_accelerator.h:85: InputBufferInfo() = default; On 2016/05/05 10:10:04, kcwu wrote: > IIUC, please put these "= default" lines in .cpp in order to avoid compiler to > generate these default methods more than once. Done. https://codereview.chromium.org/1953683002/diff/1/chrome/gpu/arc_gpu_video_de... chrome/gpu/arc_gpu_video_decode_accelerator.h:93: int32_t stride; On 2016/05/05 10:10:05, kcwu wrote: > = 0 Done. https://codereview.chromium.org/1953683002/diff/1/components/arc/common/video... File components/arc/common/video_accelerator.mojom (right): https://codereview.chromium.org/1953683002/diff/1/components/arc/common/video... components/arc/common/video_accelerator.mojom:64: BindDmabuf@2(PortType port, uint32 index, handle dmabuf_fd, int32 stride); On 2016/05/05 10:10:05, kcwu wrote: > uint32 But it is int32_t in where it's used: https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/native_pixm... In addition, according to video_frame.h, it could be negative: https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_f...
https://codereview.chromium.org/1953683002/diff/20001/chrome/gpu/arc_gpu_vide... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1953683002/diff/20001/chrome/gpu/arc_gpu_vide... chrome/gpu/arc_gpu_video_decode_accelerator.cc:375: base::FileDescriptor(info.handle.release(), true); (My question about how this fd ends up getting freed from the other CL applies here as well =) https://codereview.chromium.org/1953683002/diff/20001/chrome/gpu/arc_gpu_vide... chrome/gpu/arc_gpu_video_decode_accelerator.cc:378: std::vector<gfx::GpuMemoryBufferHandle> buffers; I have good news. Just write: std::vector<gfx::GpuMemoryBufferHandle> buffers{handle}; https://codereview.chromium.org/1953683002/diff/20001/components/arc/common/v... File components/arc/common/video_accelerator.mojom (right): https://codereview.chromium.org/1953683002/diff/20001/components/arc/common/v... components/arc/common/video_accelerator.mojom:64: BindDmabuf@2(PortType port, uint32 index, handle dmabuf_fd, int32 stride); Why int32 and not uint32?
https://codereview.chromium.org/1953683002/diff/20001/chrome/gpu/arc_gpu_vide... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1953683002/diff/20001/chrome/gpu/arc_gpu_vide... chrome/gpu/arc_gpu_video_decode_accelerator.cc:375: base::FileDescriptor(info.handle.release(), true); On 2016/05/07 06:22:56, dcheng wrote: > (My question about how this fd ends up getting freed from the other CL applies > here as well =) As the reply in the other CL. The fd will be managed by ScopedFD in the implementation of the ImportBufferForPicture(). https://codereview.chromium.org/1953683002/diff/20001/chrome/gpu/arc_gpu_vide... chrome/gpu/arc_gpu_video_decode_accelerator.cc:378: std::vector<gfx::GpuMemoryBufferHandle> buffers; On 2016/05/07 06:22:56, dcheng wrote: > I have good news. Just write: > > std::vector<gfx::GpuMemoryBufferHandle> buffers{handle}; Thanks. It's really nice. https://codereview.chromium.org/1953683002/diff/20001/components/arc/common/v... File components/arc/common/video_accelerator.mojom (right): https://codereview.chromium.org/1953683002/diff/20001/components/arc/common/v... components/arc/common/video_accelerator.mojom:64: BindDmabuf@2(PortType port, uint32 index, handle dmabuf_fd, int32 stride); On 2016/05/07 06:22:56, dcheng wrote: > Why int32 and not uint32? It is int32_t in where it's used: https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/native_pixm... In addition, according to video_frame.h, stride could be negative: https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_f...
https://codereview.chromium.org/1953683002/diff/40001/chrome/gpu/arc_gpu_vide... File chrome/gpu/arc_gpu_video_decode_accelerator.h (right): https://codereview.chromium.org/1953683002/diff/40001/chrome/gpu/arc_gpu_vide... chrome/gpu/arc_gpu_video_decode_accelerator.h:46: int32_t stride) override; size_t? https://codereview.chromium.org/1953683002/diff/40001/chrome/gpu/arc_gpu_vide... chrome/gpu/arc_gpu_video_decode_accelerator.h:82: off_t offset = 0; Please add documentation https://codereview.chromium.org/1953683002/diff/40001/chrome/gpu/arc_gpu_vide... chrome/gpu/arc_gpu_video_decode_accelerator.h:90: // The information about the Dmabuf used as an output buffer. s/Dmabuf/dmabuf/ https://codereview.chromium.org/1953683002/diff/40001/chrome/gpu/arc_gpu_vide... chrome/gpu/arc_gpu_video_decode_accelerator.h:148: // be used yet. Will call VDA::ImportBufferForPicture() when those buffers are s/not be used yet/have not been passed to VDA yet/ https://codereview.chromium.org/1953683002/diff/40001/chrome/gpu/arc_video_ac... File chrome/gpu/arc_video_accelerator.h (right): https://codereview.chromium.org/1953683002/diff/40001/chrome/gpu/arc_video_ac... chrome/gpu/arc_video_accelerator.h:125: virtual void BindDmabuf(PortType port, Please add documentation for stride (esp. units).
owenlin@chromium.org changed reviewers: + scherkus@chromium.org
Hi scherkus@, we are trying to add stride for the output buffers used in ARC++. According to https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_f... The value could be negative, can you comment on when this is going to happen?
https://codereview.chromium.org/1953683002/diff/1/chrome/gpu/arc_video_accele... File chrome/gpu/arc_video_accelerator.h (right): https://codereview.chromium.org/1953683002/diff/1/chrome/gpu/arc_video_accele... chrome/gpu/arc_video_accelerator.h:128: int32_t stride) = 0; On 2016/05/05 10:10:05, kcwu wrote: > add comments that stride is counting in bytes. this is not addressed yet.
On 2016/05/09 08:04:54, Owen Lin wrote: > Hi scherkus@, we are trying to add stride for the output buffers used in ARC++. > > According to > > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_f... > > The value could be negative, can you comment on when this is going to happen? I haven't worked on Chrome for over a year and to be honest the reason completely escapes me. Perhaps xhwang@ or dalecurtis@ would know?
PTAL. Thanks. https://codereview.chromium.org/1953683002/diff/1/chrome/gpu/arc_video_accele... File chrome/gpu/arc_video_accelerator.h (right): https://codereview.chromium.org/1953683002/diff/1/chrome/gpu/arc_video_accele... chrome/gpu/arc_video_accelerator.h:128: int32_t stride) = 0; On 2016/05/09 08:54:30, kcwu wrote: > On 2016/05/05 10:10:05, kcwu wrote: > > add comments that stride is counting in bytes. > > this is not addressed yet. Done. https://codereview.chromium.org/1953683002/diff/40001/chrome/gpu/arc_gpu_vide... File chrome/gpu/arc_gpu_video_decode_accelerator.h (right): https://codereview.chromium.org/1953683002/diff/40001/chrome/gpu/arc_gpu_vide... chrome/gpu/arc_gpu_video_decode_accelerator.h:46: int32_t stride) override; On 2016/05/09 07:34:53, Pawel Osciak wrote: > size_t? As we discussed offline, use int32_t to match the stride in NativePixmapHandle. https://codereview.chromium.org/1953683002/diff/40001/chrome/gpu/arc_gpu_vide... chrome/gpu/arc_gpu_video_decode_accelerator.h:82: off_t offset = 0; On 2016/05/09 07:34:53, Pawel Osciak wrote: > Please add documentation Done. https://codereview.chromium.org/1953683002/diff/40001/chrome/gpu/arc_gpu_vide... chrome/gpu/arc_gpu_video_decode_accelerator.h:90: // The information about the Dmabuf used as an output buffer. On 2016/05/09 07:34:53, Pawel Osciak wrote: > s/Dmabuf/dmabuf/ Done. https://codereview.chromium.org/1953683002/diff/40001/chrome/gpu/arc_gpu_vide... chrome/gpu/arc_gpu_video_decode_accelerator.h:148: // be used yet. Will call VDA::ImportBufferForPicture() when those buffers are On 2016/05/09 07:34:53, Pawel Osciak wrote: > s/not be used yet/have not been passed to VDA yet/ Done. https://codereview.chromium.org/1953683002/diff/40001/chrome/gpu/arc_video_ac... File chrome/gpu/arc_video_accelerator.h (right): https://codereview.chromium.org/1953683002/diff/40001/chrome/gpu/arc_video_ac... chrome/gpu/arc_video_accelerator.h:125: virtual void BindDmabuf(PortType port, On 2016/05/09 07:34:53, Pawel Osciak wrote: > Please add documentation for stride (esp. units). Done.
lgtm https://codereview.chromium.org/1953683002/diff/80001/chrome/gpu/arc_gpu_vide... File chrome/gpu/arc_gpu_video_decode_accelerator.h (right): https://codereview.chromium.org/1953683002/diff/80001/chrome/gpu/arc_gpu_vide... chrome/gpu/arc_gpu_video_decode_accelerator.h:86: // The length of the payload. size of the payload in bytes.
Hi dcheng@, need your approval for the mojom file. PTAL. Thanks.
I don't think my question about stride has been answered yet: what happens if it's negative? Is that a sensible value?
On 2016/05/12 02:52:25, dcheng wrote: > I don't think my question about stride has been answered yet: what happens if > it's negative? Is that a sensible value? We have discussed about it off-line with Pawel. The stride could be wrong in many ways, negative, 0, or too big. I think we depends on the driver to access only the valid range of the dmabuf. So when the stride is wrong, we will get a distorted image and that's all. Here, we don't have enough knowledge to know it is valid or not. But it should not be negative in our cases. I will add a check to report error when it's negative. How do you think?
dcheng@chromium.org changed reviewers: + rickyz@chromium.org
On 2016/05/12 at 06:13:14, owenlin wrote: > On 2016/05/12 02:52:25, dcheng wrote: > > I don't think my question about stride has been answered yet: what happens if > > it's negative? Is that a sensible value? > > We have discussed about it off-line with Pawel. The stride could be wrong in many ways, negative, 0, or too big. > I think we depends on the driver to access only the valid range of the dmabuf. > So when the stride is wrong, we will get a distorted image and that's all. > > Here, we don't have enough knowledge to know it is valid or not. But it should not be negative in our cases. > I will add a check to report error when it's negative. How do you think? Is the driver actually resilient to bad values of stride? I'm guessing this must have come up in IPC reviews in the past? +rickyz@, I know I should consider ARC++ less privileged than the browser process... but what about compared to the GPU process?
On 2016/05/13 06:06:42, dcheng wrote: > On 2016/05/12 at 06:13:14, owenlin wrote: > > On 2016/05/12 02:52:25, dcheng wrote: > > > I don't think my question about stride has been answered yet: what happens > if > > > it's negative? Is that a sensible value? > > > > We have discussed about it off-line with Pawel. The stride could be wrong in > many ways, negative, 0, or too big. > > I think we depends on the driver to access only the valid range of the dmabuf. > > So when the stride is wrong, we will get a distorted image and that's all. > > > > Here, we don't have enough knowledge to know it is valid or not. But it should > not be negative in our cases. > > I will add a check to report error when it's negative. How do you think? > > Is the driver actually resilient to bad values of stride? I'm guessing this must > have come up in IPC reviews in the past? I think that really depends on the drivers. On ARM, the value is just ignored. Maybe posciak@, or tfiga@ can comments on this. I think we need to pass the same value for rendering, right? > > +rickyz@, I know I should consider ARC++ less privileged than the browser > process... but what about compared to the GPU process? I believe ARC++ is still less privileged than the GPU process.
owenlin@chromium.org changed reviewers: + tfiga@chromium.org
Description was changed from ========== Add stride for imported Dmabuf in ArcVideoAccelerator. We need the stride information so that the hardware codec knows how to fill the buffer. BUG=None. TEST=Play video in YouTube and ApiDemos on ARC ========== to ========== Add stride for imported Dmabuf in ArcVideoAccelerator. We need the stride information so that the hardware codec knows how to fill the buffer. BUG=None. TEST=Play video in YouTube and ApiDemos on ARC ==========
On 2016/05/13 at 06:50:20, owenlin wrote: > On 2016/05/13 06:06:42, dcheng wrote: > > On 2016/05/12 at 06:13:14, owenlin wrote: > > > On 2016/05/12 02:52:25, dcheng wrote: > > > > I don't think my question about stride has been answered yet: what happens > > if > > > > it's negative? Is that a sensible value? > > > > > > We have discussed about it off-line with Pawel. The stride could be wrong in > > many ways, negative, 0, or too big. > > > I think we depends on the driver to access only the valid range of the dmabuf. > > > So when the stride is wrong, we will get a distorted image and that's all. > > > > > > Here, we don't have enough knowledge to know it is valid or not. But it should > > not be negative in our cases. > > > I will add a check to report error when it's negative. How do you think? > > > > Is the driver actually resilient to bad values of stride? I'm guessing this must > > have come up in IPC reviews in the past? > > I think that really depends on the drivers. On ARM, the value is just ignored. > Maybe posciak@, or tfiga@ can comments on this. > > I think we need to pass the same value for rendering, right? That doesn't feel sufficient to me: what if the stride is overly large? Can we use a trusted source to find the length of the DMA buffer and verify that the stride is sane given the size of the buffer? > > > > > +rickyz@, I know I should consider ARC++ less privileged than the browser > > process... but what about compared to the GPU process? > > I believe ARC++ is still less privileged than the GPU process.
On 2016/05/13 21:36:51, dcheng wrote: > On 2016/05/13 at 06:50:20, owenlin wrote: > > On 2016/05/13 06:06:42, dcheng wrote: > > > On 2016/05/12 at 06:13:14, owenlin wrote: > > > > On 2016/05/12 02:52:25, dcheng wrote: > > > > > I don't think my question about stride has been answered yet: what > happens > > > if > > > > > it's negative? Is that a sensible value? > > > > > > > > We have discussed about it off-line with Pawel. The stride could be wrong > in > > > many ways, negative, 0, or too big. > > > > I think we depends on the driver to access only the valid range of the > dmabuf. > > > > So when the stride is wrong, we will get a distorted image and that's all. > > > > > > > > Here, we don't have enough knowledge to know it is valid or not. But it > should > > > not be negative in our cases. > > > > I will add a check to report error when it's negative. How do you think? > > > > > > Is the driver actually resilient to bad values of stride? I'm guessing this > must > > > have come up in IPC reviews in the past? > > > > I think that really depends on the drivers. On ARM, the value is just ignored. > > Maybe posciak@, or tfiga@ can comments on this. > > > > I think we need to pass the same value for rendering, right? > > That doesn't feel sufficient to me: what if the stride is overly large? Can we > use a trusted source to find the length of the DMA buffer and verify that the > stride is sane given the size of the buffer? Agree. I believe we cannot trust the driver. Thank you for the suggestion. I add code to verify the length of stride. PTAL. > > > > > > > > > +rickyz@, I know I should consider ARC++ less privileged than the browser > > > process... but what about compared to the GPU process? > > > > I believe ARC++ is still less privileged than the GPU process.
https://codereview.chromium.org/1953683002/diff/140001/chrome/gpu/arc_gpu_vid... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1953683002/diff/140001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:178: height = height * 3 / 2; What happens if height isn't even divisible by two after this? https://codereview.chromium.org/1953683002/diff/140001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:188: if (stride < 0 || height * stride > size) { height * stride should use CheckedNumeric: https://code.google.com/p/chromium/codesearch#chromium/src/base/numerics/safe...
https://codereview.chromium.org/1953683002/diff/140001/chrome/gpu/arc_gpu_vid... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1953683002/diff/140001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:178: height = height * 3 / 2; On 2016/05/17 01:12:28, dcheng wrote: > What happens if height isn't even divisible by two after this? The coded height should be always even for YUV frame. I added a DCHECK for it. https://codereview.chromium.org/1953683002/diff/140001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:188: if (stride < 0 || height * stride > size) { On 2016/05/17 01:12:27, dcheng wrote: > height * stride should use CheckedNumeric: > https://code.google.com/p/chromium/codesearch#chromium/src/base/numerics/safe... Thanks.
https://codereview.chromium.org/1953683002/diff/160001/chrome/gpu/arc_gpu_vid... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1953683002/diff/160001/chrome/gpu/arc_gpu_vid... chrome/gpu/arc_gpu_video_decode_accelerator.cc:179: DCHECK(height % 2 == 0); // The coded height should be even for YUV. Not DCHECK(), this should just return false.
lgtm
Description was changed from ========== Add stride for imported Dmabuf in ArcVideoAccelerator. We need the stride information so that the hardware codec knows how to fill the buffer. BUG=None. TEST=Play video in YouTube and ApiDemos on ARC ========== to ========== Add stride for imported Dmabuf in ArcVideoAccelerator. We need the stride information so that the hardware codec knows how to fill the buffer. BUG=b/28783710 TEST=Play video in YouTube and ApiDemos on ARC ==========
The CQ bit was checked by owenlin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from posciak@chromium.org, dcheng@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1953683002/#ps200001 (title: "address posciak's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1953683002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1953683002/200001
wuchengli@chromium.org changed reviewers: - wuchengli@chromium.org
Message was sent while issue was closed.
Description was changed from ========== Add stride for imported Dmabuf in ArcVideoAccelerator. We need the stride information so that the hardware codec knows how to fill the buffer. BUG=b/28783710 TEST=Play video in YouTube and ApiDemos on ARC ========== to ========== Add stride for imported Dmabuf in ArcVideoAccelerator. We need the stride information so that the hardware codec knows how to fill the buffer. BUG=b/28783710 TEST=Play video in YouTube and ApiDemos on ARC ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Add stride for imported Dmabuf in ArcVideoAccelerator. We need the stride information so that the hardware codec knows how to fill the buffer. BUG=b/28783710 TEST=Play video in YouTube and ApiDemos on ARC ========== to ========== Add stride for imported Dmabuf in ArcVideoAccelerator. We need the stride information so that the hardware codec knows how to fill the buffer. BUG=b/28783710 TEST=Play video in YouTube and ApiDemos on ARC Committed: https://crrev.com/df2a3c6dbef2976dc03827d99c0370aa479dfc5a Cr-Commit-Position: refs/heads/master@{#395285} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/df2a3c6dbef2976dc03827d99c0370aa479dfc5a Cr-Commit-Position: refs/heads/master@{#395285} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
