Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9766007:
Fix incorrect VideoFrame::row_bytes() for RGB. Add tests. (Closed)
|
Created:
8 years, 9 months ago by DaleCurtis Modified:
8 years, 9 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix incorrect VideoFrame::row_bytes() for RGB. Add tests.
Also converts the Copy*Plane methods in video_util.cc to use size_t
instead of int.
BUG=119156
TEST=Built chrome. Ran unittests.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=128379
Patch Set 1 : Typos. #
Total comments: 14
Patch Set 2 : Rework test. #
Total comments: 10
Patch Set 3 : Fixes. #Patch Set 4 : Rebase. #Messages
Total messages: 12 (0 generated)
https://chromiumcodereview.appspot.com/9766007/diff/4001/media/base/video_fra... File media/base/video_frame_unittest.cc (right): https://chromiumcodereview.appspot.com/9766007/diff/4001/media/base/video_fra... media/base/video_frame_unittest.cc:170: TEST(VideoFrame, CheckRowSizes) { A bit lengthy, but the generic version proved just as long and harder to read: http://pastebin.com/tTYQJqXS
https://chromiumcodereview.appspot.com/9766007/diff/4001/media/base/video_fra... File media/base/video_frame.cc (right): https://chromiumcodereview.appspot.com/9766007/diff/4001/media/base/video_fra... media/base/video_frame.cc:212: return width_ * 2; FTR, nothing uses these, right? (i.e. this a rake-in-the-grass waiting for someone to use it, but there's no triggerable bug here now) You might be interested in http://code.google.com/p/chromium/issues/detail?id=108306 ;) https://chromiumcodereview.appspot.com/9766007/diff/4001/media/base/video_fra... File media/base/video_frame_unittest.cc (right): https://chromiumcodereview.appspot.com/9766007/diff/4001/media/base/video_fra... media/base/video_frame_unittest.cc:127: static inline size_t RoundUp(size_t value, size_t alignment) { If inline was extraneous this could easily become a public (*ForTest()) static helper method of VideoFrame and avoid the duplication. Wanna check perf impact of this? I suspect it's nil. https://chromiumcodereview.appspot.com/9766007/diff/4001/media/base/video_fra... media/base/video_frame_unittest.cc:129: DCHECK((alignment + (alignment - 1)) == (alignment | (alignment - 1))); s/DCHECK/ASSERT_TRUE/ (this is something I'm guilty of too; chromium's test infra makes it much friendlier to assert than to dcheck) https://chromiumcodereview.appspot.com/9766007/diff/4001/media/base/video_fra... media/base/video_frame_unittest.cc:176: // Ensure each frame format yields the correct number of rows and row_bytes. This comment belongs at the top of the TEST(). But it's actually misleading IMO; most (all?) of the RoundUp business is implementation artifact for performance reasons, not an artifact of "correctness" of the frame. If we shipped on a 128-bit platform we might bump some of the rounding, but it'd be hard to justify changing the test to match with anything other than "this is what the code actually emits". IMO this test is the wrong way to go about it. Instead, to assert the values are correct, *use* the values for whatever they'd be used for by a client, and assert that the result is correct. For example, if you copied the data out of the planes into another buffer using the accessors in question to bound the loops, and then checked the resulting buffers for having the expected bits at least in the visible bytes, you'd be able to: - ensure the accessors return values large enough to account for the visible bytes - ensure the accessors don't return wildly-too-large values (b/c memcheck/asan bots would 'splode) WDYT? https://chromiumcodereview.appspot.com/9766007/diff/4001/media/base/video_uti... File media/base/video_util.cc (right): https://chromiumcodereview.appspot.com/9766007/diff/4001/media/base/video_uti... media/base/video_util.cc:13: size_t rows, VideoFrame* frame) { Are you worried about an image w/ over 2 billion in any dimension? (never use unsigned types in C++ just because the data you expect them to hold is always >=0; there be monsters in manipulating unsigned types, which is why in general we only use them for things that actually *require* unsignedness, such as: system APIs requiring size_t and bit vectors)
https://chromiumcodereview.appspot.com/9766007/diff/4001/media/base/video_fra... File media/base/video_frame.cc (right): https://chromiumcodereview.appspot.com/9766007/diff/4001/media/base/video_fra... media/base/video_frame.cc:212: return width_ * 2; On 2012/03/21 04:33:44, Ami Fischman wrote: > FTR, nothing uses these, right? > (i.e. this a rake-in-the-grass waiting for someone to use it, but there's no > triggerable bug here now) > > You might be interested in > http://code.google.com/p/chromium/issues/detail?id=108306 ;) None that I could find. I like the rake in the grass analogy :) After seeing that bug I did a little more digging and I'm pretty sure all of the formats are being used at least superficially in one way or another in tests, remoting, decoding, etc. I'm not sure we can pare this down without creating separate enums and refactoring everyone. Why are we not just using WebKit::WebVideoFrame directly here? https://chromiumcodereview.appspot.com/9766007/diff/4001/media/base/video_fra... File media/base/video_frame_unittest.cc (right): https://chromiumcodereview.appspot.com/9766007/diff/4001/media/base/video_fra... media/base/video_frame_unittest.cc:127: static inline size_t RoundUp(size_t value, size_t alignment) { On 2012/03/21 04:33:44, Ami Fischman wrote: > If inline was extraneous this could easily become a public (*ForTest()) static > helper method of VideoFrame and avoid the duplication. > Wanna check perf impact of this? I suspect it's nil. Aside from the DCHECK it's just a simple return method, is it bad form to static inline the whole thing in the header file? The new test doesn't use this, so I've left it alone. https://chromiumcodereview.appspot.com/9766007/diff/4001/media/base/video_fra... media/base/video_frame_unittest.cc:176: // Ensure each frame format yields the correct number of rows and row_bytes. On 2012/03/21 04:33:44, Ami Fischman wrote: > This comment belongs at the top of the TEST(). > But it's actually misleading IMO; most (all?) of the RoundUp business is > implementation artifact for performance reasons, not an artifact of > "correctness" of the frame. If we shipped on a 128-bit platform we might bump > some of the rounding, but it'd be hard to justify changing the test to match > with anything other than "this is what the code actually emits". > > IMO this test is the wrong way to go about it. > Instead, to assert the values are correct, *use* the values for whatever they'd > be used for by a client, and assert that the result is correct. > For example, if you copied the data out of the planes into another buffer using > the accessors in question to bound the loops, and then checked the resulting > buffers for having the expected bits at least in the visible bytes, you'd be > able to: > - ensure the accessors return values large enough to account for the visible > bytes > - ensure the accessors don't return wildly-too-large values (b/c memcheck/asan > bots would 'splode) > > WDYT? I think it sounds great and was similar to what I was thinking, but didn't do. I've written up a new test case which is a mix of the old and new. Let me know what you think. https://chromiumcodereview.appspot.com/9766007/diff/4001/media/base/video_uti... File media/base/video_util.cc (right): https://chromiumcodereview.appspot.com/9766007/diff/4001/media/base/video_uti... media/base/video_util.cc:13: size_t rows, VideoFrame* frame) { On 2012/03/21 04:33:44, Ami Fischman wrote: > Are you worried about an image w/ over 2 billion in any dimension? > > (never use unsigned types in C++ just because the data you expect them to hold > is always >=0; there be monsters in manipulating unsigned types, which is why in > general we only use them for things that actually *require* unsignedness, such > as: system APIs requiring size_t and bit vectors) I just flipped these to make them consistent with width and height which were already size_t. As well as keeping me from having to add casts to every ASSERT_EQ(<value>, RoundUp(...)). Which if I switch to just using the values will no longer be necessary. Since you're concerned, I'll flip them back in any case.
LGTM % nits (some of which are taste/optional) https://chromiumcodereview.appspot.com/9766007/diff/4001/media/base/video_fra... File media/base/video_frame.cc (right): https://chromiumcodereview.appspot.com/9766007/diff/4001/media/base/video_fra... media/base/video_frame.cc:212: return width_ * 2; On 2012/03/22 01:24:43, DaleCurtis wrote: > After seeing that bug I did a little more digging and I'm pretty sure all of the > formats are being used at least superficially in one way or another in tests, > remoting, decoding, etc. I'm not sure we can pare this down without creating > separate enums and refactoring everyone. Oh, sad panda. > Why are we not just using WebKit::WebVideoFrame directly here? media/ shouldn't depend on third_party/WebKit. https://chromiumcodereview.appspot.com/9766007/diff/4001/media/base/video_fra... File media/base/video_frame_unittest.cc (right): https://chromiumcodereview.appspot.com/9766007/diff/4001/media/base/video_fra... media/base/video_frame_unittest.cc:127: static inline size_t RoundUp(size_t value, size_t alignment) { On 2012/03/22 01:24:43, DaleCurtis wrote: > Aside from the DCHECK it's just a simple return method, is it bad form to static > inline the whole thing in the header file? Yes. > The new test doesn't use this, so > I've left it alone. Cool. https://chromiumcodereview.appspot.com/9766007/diff/11001/media/base/video_fr... File media/base/video_frame_unittest.cc (right): https://chromiumcodereview.appspot.com/9766007/diff/11001/media/base/video_fr... media/base/video_frame_unittest.cc:171: const base::TimeDelta kDurationA = base::TimeDelta::FromMicroseconds(1667); Drop the "A" from this and the previous line. https://chromiumcodereview.appspot.com/9766007/diff/11001/media/base/video_fr... media/base/video_frame_unittest.cc:175: const Tuple4<VideoFrame::Format, int, int, char *> kFrameTests[] = { s/char \*/StringPiece/ would allow you to drop the casts below (which should have been static_cast's, if you were going to keep them, but probably the type should have been const char * anyway to avoid the need to cast in the first place). https://chromiumcodereview.appspot.com/9766007/diff/11001/media/base/video_fr... media/base/video_frame_unittest.cc:192: base::MD5Digest digest; This and the next line can go near the bottom of the loop (where they are first used) https://chromiumcodereview.appspot.com/9766007/diff/11001/media/base/video_fr... media/base/video_frame_unittest.cc:194: for(unsigned long i = 0; i < arraysize(kFrameTests); i++) { If it were me I'd extract the loop body into a helper function, because that would replace the need for the kFrameTests array above (with its MakeTuple()s) with simpler function calls TestSizes(VideoFrame::RGB32, 1, 4, "de6d3d567e282f6a38d478f04fc81fb0"); etc. https://chromiumcodereview.appspot.com/9766007/diff/11001/media/base/video_fr... media/base/video_frame_unittest.cc:195: scoped_refptr<VideoFrame> frame = VideoFrame::CreateFrame( Add SCOPED_TRACE(IntToString(i)); for easier debugging.
https://chromiumcodereview.appspot.com/9766007/diff/4001/media/base/video_fra... File media/base/video_frame.cc (right): https://chromiumcodereview.appspot.com/9766007/diff/4001/media/base/video_fra... media/base/video_frame.cc:212: return width_ * 2; On 2012/03/22 01:24:43, DaleCurtis wrote: > On 2012/03/21 04:33:44, Ami Fischman wrote: > > FTR, nothing uses these, right? > > (i.e. this a rake-in-the-grass waiting for someone to use it, but there's no > > triggerable bug here now) > > > > You might be interested in > > http://code.google.com/p/chromium/issues/detail?id=108306 ;) > > None that I could find. I like the rake in the grass analogy :) > > After seeing that bug I did a little more digging and I'm pretty sure all of the > formats are being used at least superficially in one way or another in tests, > remoting, decoding, etc. I'm not sure we can pare this down without creating > separate enums and refactoring everyone. I believe all usage of these additional formats are purely superficial. Are you sure refactoring is necessary as opposed to deleting all references/tests or is that what you're referring to when you say refactoring? > Why are we not just using WebKit::WebVideoFrame directly here? That would be a layering violation.
https://chromiumcodereview.appspot.com/9766007/diff/4001/media/base/video_fra... File media/base/video_frame.cc (right): https://chromiumcodereview.appspot.com/9766007/diff/4001/media/base/video_fra... media/base/video_frame.cc:212: return width_ * 2; On 2012/03/22 10:57:54, scherkus wrote: > On 2012/03/22 01:24:43, DaleCurtis wrote: > > On 2012/03/21 04:33:44, Ami Fischman wrote: > > > FTR, nothing uses these, right? > > > (i.e. this a rake-in-the-grass waiting for someone to use it, but there's no > > > triggerable bug here now) > > > > > > You might be interested in > > > http://code.google.com/p/chromium/issues/detail?id=108306 ;) > > > > None that I could find. I like the rake in the grass analogy :) > > > > After seeing that bug I did a little more digging and I'm pretty sure all of > the > > formats are being used at least superficially in one way or another in tests, > > remoting, decoding, etc. I'm not sure we can pare this down without creating > > separate enums and refactoring everyone. > > I believe all usage of these additional formats are purely superficial. > > Are you sure refactoring is necessary as opposed to deleting all > references/tests or is that what you're referring to when you say refactoring? > > > Why are we not just using WebKit::WebVideoFrame directly here? > > That would be a layering violation. To be precise, I took my notes from the previous search on "VideoFrame::<format>" and "<format>" and did some additional tracing: Used in production, testing or tools: INVALID RGB32 RGBA YV12 YV16 EMPTY I420 NATIVE_TEXTURE Used in testing: ASCII Used superficially, but unnecessary: RGB565 RGB24 NV12 Unused (mistook for another enum on the first search): RGB555 When I said refactoring I meant if we introduced a new enum or wanted to replace usage of a format. Culling RGB565, RGB24, NV12, and RGB555 can be done easily, we'll just have to manually assign values in the enum for the gaps they leave. If that's desirable, I'll take the bug mentioned above and create another CL.
On 2012/03/22 17:30:35, DaleCurtis wrote: > https://chromiumcodereview.appspot.com/9766007/diff/4001/media/base/video_fra... > File media/base/video_frame.cc (right): > > https://chromiumcodereview.appspot.com/9766007/diff/4001/media/base/video_fra... > media/base/video_frame.cc:212: return width_ * 2; > On 2012/03/22 10:57:54, scherkus wrote: > > On 2012/03/22 01:24:43, DaleCurtis wrote: > > > On 2012/03/21 04:33:44, Ami Fischman wrote: > > > > FTR, nothing uses these, right? > > > > (i.e. this a rake-in-the-grass waiting for someone to use it, but there's > no > > > > triggerable bug here now) > > > > > > > > You might be interested in > > > > http://code.google.com/p/chromium/issues/detail?id=108306 ;) > > > > > > None that I could find. I like the rake in the grass analogy :) > > > > > > After seeing that bug I did a little more digging and I'm pretty sure all of > > the > > > formats are being used at least superficially in one way or another in > tests, > > > remoting, decoding, etc. I'm not sure we can pare this down without > creating > > > separate enums and refactoring everyone. > > > > I believe all usage of these additional formats are purely superficial. > > > > Are you sure refactoring is necessary as opposed to deleting all > > references/tests or is that what you're referring to when you say refactoring? > > > > > Why are we not just using WebKit::WebVideoFrame directly here? > > > > That would be a layering violation. > > To be precise, I took my notes from the previous search on > "VideoFrame::<format>" and "<format>" and did some additional tracing: > > Used in production, testing or tools: > INVALID > RGB32 > RGBA > YV12 > YV16 > EMPTY > I420 > NATIVE_TEXTURE > > Used in testing: > ASCII > > Used superficially, but unnecessary: > RGB565 > RGB24 > NV12 > > Unused (mistook for another enum on the first search): > RGB555 > > When I said refactoring I meant if we introduced a new enum or wanted to replace > usage of a format. Culling RGB565, RGB24, NV12, and RGB555 can be done easily, > we'll just have to manually assign values in the enum for the gaps they leave. > If that's desirable, I'll take the bug mentioned above and create another CL. If we really wanted to remove them for good we can temporarily change webvideoframe_impl.cc to use a switch for converting between WebVideoFrame::Format and VideoFrame::Format as opposed to a static_cast<> + COMPILE_ASSERT. Then it's a matter of: 1) Remove the Chromium enums + update the adapter to only reference the WebVideoFrame types we care about 2) Land a WebKit patch to remove the unnecessary enums 3) Switch back to static_cast<> + compile assert
Please move the convo about dropping enum values to the bug.
Thanks. Will rebase after the hashing CL lands and then wait for vrk's approval. https://chromiumcodereview.appspot.com/9766007/diff/11001/media/base/video_fr... File media/base/video_frame_unittest.cc (right): https://chromiumcodereview.appspot.com/9766007/diff/11001/media/base/video_fr... media/base/video_frame_unittest.cc:171: const base::TimeDelta kDurationA = base::TimeDelta::FromMicroseconds(1667); On 2012/03/22 10:53:25, Ami Fischman wrote: > Drop the "A" from this and the previous line. Done. https://chromiumcodereview.appspot.com/9766007/diff/11001/media/base/video_fr... media/base/video_frame_unittest.cc:175: const Tuple4<VideoFrame::Format, int, int, char *> kFrameTests[] = { On 2012/03/22 10:53:25, Ami Fischman wrote: > s/char \*/StringPiece/ > would allow you to drop the casts below (which should have been static_cast's, > if you were going to keep them, but probably the type should have been const > char * anyway to avoid the need to cast in the first place). Dropped all this in favor of extracted function. https://chromiumcodereview.appspot.com/9766007/diff/11001/media/base/video_fr... media/base/video_frame_unittest.cc:192: base::MD5Digest digest; On 2012/03/22 10:53:25, Ami Fischman wrote: > This and the next line can go near the bottom of the loop (where they are first > used) Done. https://chromiumcodereview.appspot.com/9766007/diff/11001/media/base/video_fr... media/base/video_frame_unittest.cc:194: for(unsigned long i = 0; i < arraysize(kFrameTests); i++) { On 2012/03/22 10:53:25, Ami Fischman wrote: > If it were me I'd extract the loop body into a helper function, because that > would replace the need for the kFrameTests array above (with its MakeTuple()s) > with simpler function calls > TestSizes(VideoFrame::RGB32, 1, 4, "de6d3d567e282f6a38d478f04fc81fb0"); > etc. Done. https://chromiumcodereview.appspot.com/9766007/diff/11001/media/base/video_fr... media/base/video_frame_unittest.cc:195: scoped_refptr<VideoFrame> frame = VideoFrame::CreateFrame( On 2012/03/22 10:53:25, Ami Fischman wrote: > Add > SCOPED_TRACE(IntToString(i)); > for easier debugging. Done.
Got approval from vrk via chat. Pushing.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/9766007/18005
Change committed as 128379 |
