Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
178133005:
Audit/fix use of media::VideoFrame::coded_size() (Closed)
|
Created:
6 years, 10 months ago by sheu Modified:
6 years, 6 months ago CC:
chromium-reviews, hclam+watch_chromium.org, pwestin+watch_google.com, mikhal+watch_chromium.org, fischman+watch_chromium.org, jasonroberts+watch_google.com, jam, mcasas+watch_chromium.org, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, hguihot+watch_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, cc-bugs_chromium.org, wjia+watch_chromium.org, hubbe+watch_chromium.org, miu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAudit/fix use of media::VideoFrame::coded_size()
Add comments to media::VideoFrame about the meaning and use of coded_size(),
visible_rect(), and natural_size(), and audit Chrome codebase to make sure
uses of VideoFrame are consistent with this. This is in regards mostly to
making sure that odd-width/height subsampled YUV format are handled correctly.
BUG=344719
TEST=local build, run on CrOS snow, desktop Linux
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259814
Patch Set 1 : c144dba5 Initial. #Patch Set 2 : 5c04e7a7 DCHECK fix. #
Total comments: 16
Patch Set 3 : 6f577dde Comments. #
Total comments: 1
Patch Set 4 : fc39d405 AVDA bit. #Patch Set 5 : d4e1a5bd Uhh yeah. #
Total comments: 1
Patch Set 6 : e6f9affb danakj@ comments. #
Total comments: 10
Patch Set 7 : 5c698210 Comments. #Patch Set 8 : 0e183cef9 Rebase. #Patch Set 9 : dd917c40 Rebase. #Patch Set 10 : b7828c1a Rebase. #Patch Set 11 : e0048b6f make Android happy. #Patch Set 12 : 2121885f Rebase. #
Total comments: 1
Messages
Total messages: 46 (0 generated)
fischman@: PTAL. Most important are the changes on media/base/video_frame.*, in which we start checking for IsValidConfig() in WrapExternalPackedMemory() and WrapExternalYuvData(). Since this is wrapping external memory, I think it's worth it to do extra checking here.
fischman@: PTAL, thanks!
Sorry for delayed review; vacation+catchup got in the way. https://chromiumcodereview.appspot.com/178133005/diff/70001/content/common/gp... File content/common/gpu/media/android_video_encode_accelerator.cc (right): https://chromiumcodereview.appspot.com/178133005/diff/70001/content/common/gp... content/common/gpu/media/android_video_encode_accelerator.cc:212: frame->coded_size().width() == frame->visible_rect().right(), this is a weaker check than before (e.g. a right-aligned visible subrect will pass the new test but not the old). why? https://chromiumcodereview.appspot.com/178133005/diff/70001/media/base/video_... File media/base/video_frame.cc (right): https://chromiumcodereview.appspot.com/178133005/diff/70001/media/base/video_... media/base/video_frame.cc:29: // ourselves), we can pad the requested |coded_size| if necessary. "necessary" for what? https://chromiumcodereview.appspot.com/178133005/diff/70001/media/base/video_... media/base/video_frame.cc:30: gfx::Size new_coded_size(coded_size); nit: when making a change like this I prefer to rename the function parameter (e.g. orig_coded_size) and name the new thing the old name (e.g. this local var would be coded_size) to ensure no old uses remain. https://chromiumcodereview.appspot.com/178133005/diff/70001/media/base/video_... media/base/video_frame.cc:43: return NULL; Is there not enough __noreturn__ whateverness on LOG(FATAL) that this can be omitted? https://chromiumcodereview.appspot.com/178133005/diff/70001/media/base/video_... media/base/video_frame.cc:103: if (coded_size.height() % 2 != 0) I almost want to CHECK this... https://chromiumcodereview.appspot.com/178133005/diff/70001/media/base/video_... media/base/video_frame.cc:107: if (coded_size.width() % 2 != 0) I'd really like for this to be run through the "odd videos" test battery but I'm not sure what that went. The search is on in go/aqzkm https://chromiumcodereview.appspot.com/178133005/diff/70001/media/base/video_... media/base/video_frame.cc:109: case VideoFrame::UNKNOWN: should be unreachable. https://chromiumcodereview.appspot.com/178133005/diff/70001/media/base/video_... media/base/video_frame.cc:457: DCHECK_LE(visible_rect_.height(), coded_size_.height()); These last 4 are part of IsValidConfig(), so why DCHECK them again? Further, l.452 implies that being UNKNOWN makes it ok to not be IsValid, but these 4 lines say you can be UNKNOWN and invalid, but you better not be invalid in these 4 ways?!? I suspect the only legit way to get here with UNKNOWN is CreateEOSFrame(), so maybe just DCHECK for that?
https://chromiumcodereview.appspot.com/178133005/diff/70001/content/common/gp... File content/common/gpu/media/android_video_encode_accelerator.cc (right): https://chromiumcodereview.appspot.com/178133005/diff/70001/content/common/gp... content/common/gpu/media/android_video_encode_accelerator.cc:212: frame->coded_size().width() == frame->visible_rect().right(), On 2014/02/27 01:20:12, Ami Fischman wrote: > this is a weaker check than before (e.g. a right-aligned visible subrect will > pass the new test but not the old). why? My thought was that as long as the stride matched the right side of the visible rect, we should be safe with the assumption that stride == coded_size. I'll strengthen this one. https://chromiumcodereview.appspot.com/178133005/diff/70001/media/base/video_... File media/base/video_frame.cc (right): https://chromiumcodereview.appspot.com/178133005/diff/70001/media/base/video_... media/base/video_frame.cc:29: // ourselves), we can pad the requested |coded_size| if necessary. On 2014/02/27 01:20:12, Ami Fischman wrote: > "necessary" for what? Added commentary. https://chromiumcodereview.appspot.com/178133005/diff/70001/media/base/video_... media/base/video_frame.cc:30: gfx::Size new_coded_size(coded_size); On 2014/02/27 01:20:12, Ami Fischman wrote: > nit: when making a change like this I prefer to rename the function parameter > (e.g. orig_coded_size) and name the new thing the old name (e.g. this local var > would be coded_size) to ensure no old uses remain. I like to make the header declaration and the function definition match up. As before, let me know if this really needs to change. https://chromiumcodereview.appspot.com/178133005/diff/70001/media/base/video_... media/base/video_frame.cc:43: return NULL; On 2014/02/27 01:20:12, Ami Fischman wrote: > Is there not enough __noreturn__ whateverness on LOG(FATAL) that this can be > omitted? Nope. Still has to return something. https://chromiumcodereview.appspot.com/178133005/diff/70001/media/base/video_... media/base/video_frame.cc:103: if (coded_size.height() % 2 != 0) On 2014/02/27 01:20:12, Ami Fischman wrote: > I almost want to CHECK this... Well, for the cases where we only DCHECK(IsValidConfig()), we'd get the same behavior. For the cases where we're concerned enough to test it directly (WrapExternalPackedMemory() and WrapExternalYuvData()), we have the more useful return case of returning NULL. So we're covered. https://chromiumcodereview.appspot.com/178133005/diff/70001/media/base/video_... media/base/video_frame.cc:109: case VideoFrame::UNKNOWN: On 2014/02/27 01:20:12, Ami Fischman wrote: > should be unreachable. With the DCHECK in the VideoFrame constructor, these are reachable. https://chromiumcodereview.appspot.com/178133005/diff/70001/media/base/video_... media/base/video_frame.cc:457: DCHECK_LE(visible_rect_.height(), coded_size_.height()); On 2014/02/27 01:20:12, Ami Fischman wrote: > These last 4 are part of IsValidConfig(), so why DCHECK them again? > Further, l.452 implies that being UNKNOWN makes it ok to not be IsValid, but > these 4 lines say you can be UNKNOWN and invalid, but you better not be invalid > in these 4 ways?!? > > I suspect the only legit way to get here with UNKNOWN is CreateEOSFrame(), so > maybe just DCHECK for that? Gonna fix up the check paths a bit.
LGTM % AVDA comment. If you disagree then lets discuss before you land. https://chromiumcodereview.appspot.com/178133005/diff/70001/media/base/video_... File media/base/video_frame.cc (right): https://chromiumcodereview.appspot.com/178133005/diff/70001/media/base/video_... media/base/video_frame.cc:30: gfx::Size new_coded_size(coded_size); On 2014/02/28 00:40:19, sheu wrote: > On 2014/02/27 01:20:12, Ami Fischman wrote: > > nit: when making a change like this I prefer to rename the function parameter > > (e.g. orig_coded_size) and name the new thing the old name (e.g. this local > var > > would be coded_size) to ensure no old uses remain. > > I like to make the header declaration and the function definition match up. As > before, let me know if this really needs to change. Meh. (your way is better for cleaner signatures, my way is better for less chance of future bugs) https://chromiumcodereview.appspot.com/178133005/diff/110001/content/common/g... File content/common/gpu/media/android_video_encode_accelerator.cc (right): https://chromiumcodereview.appspot.com/178133005/diff/110001/content/common/g... content/common/gpu/media/android_video_encode_accelerator.cc:212: frame->coded_size().width() == frame->visible_rect().width(), On 2014/02/28 00:40:19, sheu wrote: > On 2014/02/27 01:20:12, Ami Fischman wrote: > > this is a weaker check than before (e.g. a right-aligned visible subrect will > > pass the new test but not the old). why? > > My thought was that as long as the stride matched the right side of the visible > rect, we should be safe with the assumption that stride == coded_size. > > I'll strengthen this one. I still think this is wrong because there's no way to communicate visible_rect to MediaCodec. If there are extra coded rows (for example), they'll end up visible in the encoded result. Ditto for left margins that are excluded from a visible_rect that is right-aligned with coded_size; they'll end up visible too.
I was hoping to preserve odd-height support for AVEA. Not going to hold up a CL for it though. Fixed! danakj@: PTAL at cc/*, thanks!
https://codereview.chromium.org/178133005/diff/170001/cc/resources/video_reso... File cc/resources/video_resource_updater.cc (left): https://codereview.chromium.org/178133005/diff/170001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:95: if (output_resource_format == kYUVResourceFormat) { Can you keep this method, and just have it call VideoFrame::PlaneSize instead of doing the switches?
danakj@: updated, PTAL. fischman@: I left some things out of the patchset you LGTMed :-(. Please diff against patchset 4 for the relevant changes. Note in particular the change in ffmpeg_common.c that is an actual functional change here.
cc LGTM https://chromiumcodereview.appspot.com/178133005/diff/190001/cc/resources/vid... File cc/resources/video_resource_updater.cc (right): https://chromiumcodereview.appspot.com/178133005/diff/190001/cc/resources/vid... cc/resources/video_resource_updater.cc:93: size_t plane) { prefer keep this as |plane_index| https://chromiumcodereview.appspot.com/178133005/diff/190001/cc/resources/vid... cc/resources/video_resource_updater.cc:97: } else { no need for else {} since the if{returns}.
https://chromiumcodereview.appspot.com/178133005/diff/190001/media/base/video... File media/base/video_frame.cc (right): https://chromiumcodereview.appspot.com/178133005/diff/190001/media/base/video... media/base/video_frame.cc:102: RoundUp(visible_rect.bottom(), 2)) Subtle! So it's ok for coded size to be odd as long as visible_rect is away from the oddness? The relevant codecs specify how to deal with this? https://chromiumcodereview.appspot.com/178133005/diff/190001/media/base/video... File media/base/video_frame_unittest.cc (left): https://chromiumcodereview.appspot.com/178133005/diff/190001/media/base/video... media/base/video_frame_unittest.cc:110: if (plane == 0) { Why this change? https://chromiumcodereview.appspot.com/178133005/diff/190001/media/base/video... File media/base/video_frame_unittest.cc (right): https://chromiumcodereview.appspot.com/178133005/diff/190001/media/base/video... media/base/video_frame_unittest.cc:206: ExpectFrameExtents(VideoFrame::YV16, 3, "cce408a044b212db42a10dfec304b3ef"); why these changes? (does this mean a bug was hiding in the checksum before?)
Updated for fischman@. https://chromiumcodereview.appspot.com/178133005/diff/190001/cc/resources/vid... File cc/resources/video_resource_updater.cc (right): https://chromiumcodereview.appspot.com/178133005/diff/190001/cc/resources/vid... cc/resources/video_resource_updater.cc:93: size_t plane) { On 2014/02/28 22:28:25, danakj wrote: > prefer keep this as |plane_index| Done. https://chromiumcodereview.appspot.com/178133005/diff/190001/cc/resources/vid... cc/resources/video_resource_updater.cc:97: } else { On 2014/02/28 22:28:25, danakj wrote: > no need for else {} since the if{returns}. Done. https://chromiumcodereview.appspot.com/178133005/diff/190001/media/base/video... File media/base/video_frame.cc (right): https://chromiumcodereview.appspot.com/178133005/diff/190001/media/base/video... media/base/video_frame.cc:102: RoundUp(visible_rect.bottom(), 2)) On 2014/02/28 23:17:32, Ami Fischman wrote: > Subtle! > So it's ok for coded size to be odd as long as visible_rect is away from the > oddness? The relevant codecs specify how to deal with this? The relevant codecs already allow for odd-sized coded_size (e.g. ffmpeg may choose to output an odd width/height _and_ coded_width/coded_height). So yes, if we keep visible_rect away from it we should be safe. https://chromiumcodereview.appspot.com/178133005/diff/190001/media/base/video... File media/base/video_frame_unittest.cc (left): https://chromiumcodereview.appspot.com/178133005/diff/190001/media/base/video... media/base/video_frame_unittest.cc:110: if (plane == 0) { On 2014/02/28 23:17:32, Ami Fischman wrote: > Why this change? VideoFrame::CreateFrame() can pad the coded_size of the frame, which makes frame->rows(plane) not necessarily correspond to kHeight; similarly with frame->row_bytes(plane). https://chromiumcodereview.appspot.com/178133005/diff/190001/media/base/video... File media/base/video_frame_unittest.cc (right): https://chromiumcodereview.appspot.com/178133005/diff/190001/media/base/video... media/base/video_frame_unittest.cc:206: ExpectFrameExtents(VideoFrame::YV16, 3, "cce408a044b212db42a10dfec304b3ef"); On 2014/02/28 23:17:32, Ami Fischman wrote: > why these changes? > (does this mean a bug was hiding in the checksum before?) Nope; we're padding out coded_size now so there's extra bytes getting checksummed. (The extra bytes are getting filled properly, so there's no indeterminism here.)
LGTM On Fri, Feb 28, 2014 at 5:56 PM, <sheu@chromium.org> wrote: > Updated for fischman@. > > > > https://chromiumcodereview.appspot.com/178133005/diff/ > 190001/cc/resources/video_resource_updater.cc > File cc/resources/video_resource_updater.cc (right): > > https://chromiumcodereview.appspot.com/178133005/diff/ > 190001/cc/resources/video_resource_updater.cc#newcode93 > cc/resources/video_resource_updater.cc:93: size_t plane) { > On 2014/02/28 22:28:25, danakj wrote: > >> prefer keep this as |plane_index| >> > > Done. > > > https://chromiumcodereview.appspot.com/178133005/diff/ > 190001/cc/resources/video_resource_updater.cc#newcode97 > cc/resources/video_resource_updater.cc:97: } else { > On 2014/02/28 22:28:25, danakj wrote: > >> no need for else {} since the if{returns}. >> > > Done. > > > https://chromiumcodereview.appspot.com/178133005/diff/ > 190001/media/base/video_frame.cc > File media/base/video_frame.cc (right): > > https://chromiumcodereview.appspot.com/178133005/diff/ > 190001/media/base/video_frame.cc#newcode102 > media/base/video_frame.cc:102: RoundUp(visible_rect.bottom(), 2)) > On 2014/02/28 23:17:32, Ami Fischman wrote: > >> Subtle! >> So it's ok for coded size to be odd as long as visible_rect is away >> > from the > >> oddness? The relevant codecs specify how to deal with this? >> > > The relevant codecs already allow for odd-sized coded_size (e.g. ffmpeg > may choose to output an odd width/height _and_ > coded_width/coded_height). So yes, if we keep visible_rect away from it > we should be safe. > > > https://chromiumcodereview.appspot.com/178133005/diff/ > 190001/media/base/video_frame_unittest.cc > File media/base/video_frame_unittest.cc (left): > > https://chromiumcodereview.appspot.com/178133005/diff/ > 190001/media/base/video_frame_unittest.cc#oldcode110 > media/base/video_frame_unittest.cc:110: if (plane == 0) { > On 2014/02/28 23:17:32, Ami Fischman wrote: > >> Why this change? >> > > VideoFrame::CreateFrame() can pad the coded_size of the frame, which > makes frame->rows(plane) not necessarily correspond to kHeight; > similarly with frame->row_bytes(plane). > > > https://chromiumcodereview.appspot.com/178133005/diff/ > 190001/media/base/video_frame_unittest.cc > File media/base/video_frame_unittest.cc (right): > > https://chromiumcodereview.appspot.com/178133005/diff/ > 190001/media/base/video_frame_unittest.cc#newcode206 > media/base/video_frame_unittest.cc:206: > ExpectFrameExtents(VideoFrame::YV16, 3, > "cce408a044b212db42a10dfec304b3ef"); > On 2014/02/28 23:17:32, Ami Fischman wrote: > >> why these changes? >> (does this mean a bug was hiding in the checksum before?) >> > > Nope; we're padding out coded_size now so there's extra bytes getting > checksummed. (The extra bytes are getting filled properly, so there's > no indeterminism here.) > > https://chromiumcodereview.appspot.com/178133005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by sheu@chromium.org
The CQ bit was unchecked by sheu@chromium.org
The CQ bit was checked by sheu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sheu@chromium.org/178133005/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_clang_dbg
On 2014/03/21 00:54:58, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > tryserver.chromium on linux_chromium_clang_dbg +1! Thanks for doing this!
The CQ bit was checked by sheu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sheu@chromium.org/178133005/280001
The CQ bit was unchecked by sheu@chromium.org
Found the bug. Fixed in:
https://chromiumcodereview.appspot.com/208653009/
will retry this after that lands.
The CQ bit was checked by sheu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sheu@chromium.org/178133005/280001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for media/cast/video_sender/external_video_encoder.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file media/cast/video_sender/external_video_encoder.cc
Hunk #1 FAILED at 157.
1 out of 1 hunk FAILED -- saving rejects to file
media/cast/video_sender/external_video_encoder.cc.rej
Patch: media/cast/video_sender/external_video_encoder.cc
Index: media/cast/video_sender/external_video_encoder.cc
diff --git a/media/cast/video_sender/external_video_encoder.cc
b/media/cast/video_sender/external_video_encoder.cc
index
4f97dace111f4103fdf5d8c51f6cef82498083c1..d6e4bf4a225bb0c2ee32ed6ba6504de8e6db033c
100644
--- a/media/cast/video_sender/external_video_encoder.cc
+++ b/media/cast/video_sender/external_video_encoder.cc
@@ -157,15 +157,15 @@ class LocalVideoEncodeAcceleratorClient
// Do a stride copy of the input frame to match the input requirements.
media::CopyYPlane(video_frame->data(VideoFrame::kYPlane),
video_frame->stride(VideoFrame::kYPlane),
- video_frame->natural_size().height(),
+ video_frame->coded_size().height(),
frame.get());
media::CopyUPlane(video_frame->data(VideoFrame::kUPlane),
video_frame->stride(VideoFrame::kUPlane),
- video_frame->natural_size().height(),
+ video_frame->coded_size().height(),
frame.get());
media::CopyVPlane(video_frame->data(VideoFrame::kVPlane),
video_frame->stride(VideoFrame::kVPlane),
- video_frame->natural_size().height(),
+ video_frame->coded_size().height(),
frame.get());
encoded_frame_data_storage_.push_back(
The CQ bit was checked by sheu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sheu@chromium.org/178133005/300001
The CQ bit was unchecked by commit-bot@chromium.org
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
The CQ bit was checked by sheu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sheu@chromium.org/178133005/300001
The CQ bit was unchecked by sergeyberezin@chromium.org
On 2014/03/25 18:23:01, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/sheu@chromium.org/178133005/300001 CQ got stuck on this CL, I'm unchecking the commit box. You way want to check android_dbg_triggered_tests for errors, and if you believe it's not your error, please try to check the commit box again.
The CQ bit was checked by sheu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sheu@chromium.org/178133005/520001
The CQ bit was unchecked by commit-bot@chromium.org
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
The CQ bit was checked by sheu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sheu@chromium.org/178133005/520001
Message was sent while issue was closed.
Change committed as 259814
Message was sent while issue was closed.
https://codereview.chromium.org/178133005/diff/520001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/178133005/diff/520001/media/base/video_frame.... media/base/video_frame.cc:113: RoundUp(visible_rect.bottom(), 2)) if visible_rect=coded_size this essentially requires that the size of the frame is even. Is it really necessary? Codecs should be able to handle odd video size, and it should be obvious how frames with odd sizes need to be handled when rendering them, so I'm not sure we really need this requirement.
What codec can generate odd sized encoded streams? To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2014/06/07 04:24:23, Ami GONE FROM CHROMIUM wrote: > What codec can generate odd sized encoded streams? VPX can. There is a bug for it here: crbug.com/379127. Looks like Dale already fixed it. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
