Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1286523002:
Add CHECKs when a single GpuMemoryBuffer plane is assumed (Closed)
|
Created:
5 years, 4 months ago by Andre Modified:
5 years, 4 months ago CC:
cc-bugs_chromium.org, chromium-reviews, feature-media-reviews_chromium.org, mcasas+watch_chromium.org, posciak+watch_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@420-biplanar Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd CHECKs when a single GpuMemoryBuffer plane is assumed.
Make sure Map() and GetStride() is never called with an array that is too small.
BUG=510260
TEST=No expected behavior change.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/4d4b028eab8d68f048c2e346f5564a5be9109126
Cr-Commit-Position: refs/heads/master@{#343080}
Patch Set 1 #Patch Set 2 : Fix for ericrk #Patch Set 3 : For reveman #Patch Set 4 : Forgot #include #
Total comments: 4
Patch Set 5 : DCHECK #Patch Set 6 : Rebase #
Depends on Patchset: Messages
Total messages: 26 (11 generated)
andresantoso@chromium.org changed reviewers: + ccameron@chromium.org
PTAL
On 2015/08/10 19:55:03, Andre wrote: > PTAL Seems like a good change! Two thoughts: 1) Can we add a DCHECK in video_capture_buffer_pool.cc to ensure that the call to GetStride on line 189 is safe (it is at this point, as the buffer is RGBA_8888, but the DCHECK would bring it to people's attention if this ever changes). 2) Is it worth filing a bug to re-work these functions in general - perhaps passing in the size of the buffer provided or something that would make this less error prone? Thanks!
1) Done, thanks, I missed that one. 2) reveman, WDYT?
andresantoso@chromium.org changed reviewers: + reveman@chromium.org
The CQ bit was checked by andresantoso@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/1286523002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1286523002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
There's no real issue here afaict. Number of planes is defined by the format. If anything, the format is the authority on the matter and not the "number-of-planes" utility function. Trying to protect against a broken implementation that writes out of bounds just makes the code harder to read imo. After all, we're not protected from an implementation that thinks it's fine to write to data[4]. Some DCHECKs to verify that the "number of planes" helper function returns 1 for code such as one-copy and zero-copy where the format selection is a bit more complicated sgtm. Checking that the number of planes helper returns the correct value when we explicitly use a single plane format as RGBA_8888 is overkill imo.
Thanks, PTAL. Went with CHECK instead of DCHECK, better to crash in a known place than corrupt memory.
lgtm with nits https://codereview.chromium.org/1286523002/diff/60001/cc/raster/one_copy_tile... File cc/raster/one_copy_tile_task_worker_pool.cc (right): https://codereview.chromium.org/1286523002/diff/60001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.cc:343: 1u, gfx::NumberOfPlanesForBufferFormat(gpu_memory_buffer->GetFormat())); nit: DCHECK_EQ. I don't think we should use CHECKs for things that we control. A failure here is a bug in chromium, not a platform or third party library problem. https://codereview.chromium.org/1286523002/diff/60001/cc/raster/zero_copy_til... File cc/raster/zero_copy_tile_task_worker_pool.cc (right): https://codereview.chromium.org/1286523002/diff/60001/cc/raster/zero_copy_til... cc/raster/zero_copy_tile_task_worker_pool.cc:39: 1u, gfx::NumberOfPlanesForBufferFormat(gpu_memory_buffer->GetFormat())); nit: DCHECK_EQ
https://codereview.chromium.org/1286523002/diff/60001/cc/raster/one_copy_tile... File cc/raster/one_copy_tile_task_worker_pool.cc (right): https://codereview.chromium.org/1286523002/diff/60001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.cc:343: 1u, gfx::NumberOfPlanesForBufferFormat(gpu_memory_buffer->GetFormat())); On 2015/08/11 18:59:11, reveman (OOO Aug 10 - Aug 11) wrote: > nit: DCHECK_EQ. I don't think we should use CHECKs for things that we control. A > failure here is a bug in chromium, not a platform or third party library > problem. Done. https://codereview.chromium.org/1286523002/diff/60001/cc/raster/zero_copy_til... File cc/raster/zero_copy_tile_task_worker_pool.cc (right): https://codereview.chromium.org/1286523002/diff/60001/cc/raster/zero_copy_til... cc/raster/zero_copy_tile_task_worker_pool.cc:39: 1u, gfx::NumberOfPlanesForBufferFormat(gpu_memory_buffer->GetFormat())); On 2015/08/11 18:59:11, reveman (OOO Aug 10 - Aug 11) wrote: > nit: DCHECK_EQ Done.
The CQ bit was checked by andresantoso@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org Link to the patchset: https://codereview.chromium.org/1286523002/#ps80001 (title: "DCHECK")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 1282313002 Patch 200001). Please resolve the dependency and try again.
The CQ bit was checked by andresantoso@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/1286523002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1286523002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by andresantoso@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org Link to the patchset: https://codereview.chromium.org/1286523002/#ps100001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1286523002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1286523002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/4d4b028eab8d68f048c2e346f5564a5be9109126 Cr-Commit-Position: refs/heads/master@{#343080} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
