Correct some rounding errors introduced recently.

Correct some rounding errors introduced recently.

BUG=101494
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109480

[scherkus (not reviewing), 2011-11-10 04:09:41]

few sanity checking questions but LGTM

http://codereview.chromium.org/8511043/diff/1/media/base/video_frame.cc
File media/base/video_frame.cc (right):

http://codereview.chromium.org/8511043/diff/1/media/base/video_frame.cc#newco...
media/base/video_frame.cc:121: size_t y_height = rows(VideoFrame::kYPlane);
this change looks OK and avoids over-allocating one extra row

http://codereview.chromium.org/8511043/diff/1/media/base/video_frame.cc#newco...
media/base/video_frame.cc:198: return RoundUp(width_, 2) / 2;
so we're always rounding up post-divide?

i.e., this should be equivalent to (width_ / 2) | 0x1

[Chris Evans, 2011-11-10 06:32:38]

http://codereview.chromium.org/8511043/diff/1/media/base/video_frame.cc
File media/base/video_frame.cc (right):

http://codereview.chromium.org/8511043/diff/1/media/base/video_frame.cc#newco...
media/base/video_frame.cc:198: return RoundUp(width_, 2) / 2;
On 2011/11/10 04:09:41, scherkus wrote:
> so we're always rounding up post-divide?

The intent is to round up pre-divide -- the divide is executed on the return
value of RoundUp. (The old way was to round up post-divide and that led to the
under-allocation issues).

> 
> i.e., this should be equivalent to (width_ / 2) | 0x1

More like (width_ / 2) + (width_ & 1)

The way I conceptualized these changes is to imagine a 1x1 video frame. We need
to allocate at least 1x1 of working space, even for planes that are scaled by
half on both the horiz and vert dimensions.

[Chris Evans, 2011-11-10 19:09:23]

Unless there are any objections, I'll give this a whirl in the waterfall once
the tree opens. I'll keep an eye on the memory bots :)

On 2011/11/10 06:32:38, Chris Evans wrote:
> http://codereview.chromium.org/8511043/diff/1/media/base/video_frame.cc
> File media/base/video_frame.cc (right):
> 
>
http://codereview.chromium.org/8511043/diff/1/media/base/video_frame.cc#newco...
> media/base/video_frame.cc:198: return RoundUp(width_, 2) / 2;
> On 2011/11/10 04:09:41, scherkus wrote:
> > so we're always rounding up post-divide?
> 
> The intent is to round up pre-divide -- the divide is executed on the return
> value of RoundUp. (The old way was to round up post-divide and that led to the
> under-allocation issues).
> 
> > 
> > i.e., this should be equivalent to (width_ / 2) | 0x1
> 
> More like (width_ / 2) + (width_ & 1)
> 
> The way I conceptualized these changes is to imagine a 1x1 video frame. We
need
> to allocate at least 1x1 of working space, even for planes that are scaled by
> half on both the horiz and vert dimensions.

[scherkus (not reviewing), 2011-11-10 19:11:06]

I really like your mental model of a 1x1 image.

I'll look into getting some unittests. We should be able to test and assert
bytes allocated for a variety of edge-casey things like 1x1, 1x2, 2x1, and 2x2

[Chris Evans, 2011-11-10 19:15:22]

I think we rejected anything less that 2x2 in a recent CL. Perhaps that was
specialization of this generic fix? Or perhaps we have other issues with tiny
frames :) Hard to say, but I don't think frames < 2x2 will test much against the
current code.

On 2011/11/10 19:11:06, scherkus wrote:
> I really like your mental model of a 1x1 image.
> 
> I'll look into getting some unittests. We should be able to test and assert
> bytes allocated for a variety of edge-casey things like 1x1, 1x2, 2x1, and 2x2