Allow wrapping a media::VideoFrame with a new view_rect.

Allow wrapping a media::VideoFrame with a new view_rect as long as the new view_rect is within the original rect.


BUG=349450
R=fischman@chromium.org, tommi@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255353

[perkj_chrome, 2014-03-05 15:59:26]

Hej
Can you please review?

[tommi (sloooow) - chröme, 2014-03-05 16:17:37]

lgtm with the caveat that I don't know this code well.  So, think of my lgtm as
a rubberstamp.

https://codereview.chromium.org/187573006/diff/1/media/base/video_frame_pool_...
File media/base/video_frame_pool_unittest.cc (right):

https://codereview.chromium.org/187573006/diff/1/media/base/video_frame_pool_...
media/base/video_frame_pool_unittest.cc:14: 
nit: one empty line should be enough

[Ami GONE FROM CHROMIUM, 2014-03-05 16:23:42]

https://codereview.chromium.org/187573006/diff/1/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/187573006/diff/1/media/base/video_frame.h#new...
media/base/video_frame.h:156: const gfx::Rect& visible_rect,
the impl makes me think the doco for this function wants to include:
"|visible_rect| must be a sub-rect of frame->visible_rect()."

Then the impl should actually enforce that (DCHECK).

https://codereview.chromium.org/187573006/diff/1/media/base/video_frame_pool.cc
File media/base/video_frame_pool.cc (right):

https://codereview.chromium.org/187573006/diff/1/media/base/video_frame_pool....
media/base/video_frame_pool.cc:75: pool_frame->natural_size() == natural_size) {
Should add a subrect test.

https://codereview.chromium.org/187573006/diff/1/media/base/video_frame_pool_...
File media/base/video_frame_pool_unittest.cc (right):

https://codereview.chromium.org/187573006/diff/1/media/base/video_frame_pool_...
media/base/video_frame_pool_unittest.cc:76: EXPECT_EQ(old_y_data,
new_frame->data(VideoFrame::kYPlane));
EXPECT_EQ(new_frame->visible_rect(), visible_rect);
?

[perkj_chrome, 2014-03-05 16:39:48]

Hej Thanks that was a new record for me in fast review turn arounds I think. 
I am not sure I understood all your comments Ami but I tried to address them.

https://codereview.chromium.org/187573006/diff/1/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/187573006/diff/1/media/base/video_frame.h#new...
media/base/video_frame.h:156: const gfx::Rect& visible_rect,
On 2014/03/05 16:23:42, Ami Fischman wrote:
> the impl makes me think the doco for this function wants to include:
> "|visible_rect| must be a sub-rect of frame->visible_rect()."
> 
> Then the impl should actually enforce that (DCHECK).

sub size of of codec_size() right? 

I DCHECK(IsValidConfig) which seem to validate that and more.

https://codereview.chromium.org/187573006/diff/1/media/base/video_frame_pool.cc
File media/base/video_frame_pool.cc (right):

https://codereview.chromium.org/187573006/diff/1/media/base/video_frame_pool....
media/base/video_frame_pool.cc:75: pool_frame->natural_size() == natural_size) {
On 2014/03/05 16:23:42, Ami Fischman wrote:
> Should add a subrect test.

? VideoFramePoolUnitTest::FrameReuseWithDifferentVisibleRect or did I
missunderstand?

https://codereview.chromium.org/187573006/diff/1/media/base/video_frame_pool_...
File media/base/video_frame_pool_unittest.cc (right):

https://codereview.chromium.org/187573006/diff/1/media/base/video_frame_pool_...
media/base/video_frame_pool_unittest.cc:14: 
On 2014/03/05 16:17:37, tommi wrote:
> nit: one empty line should be enough

Done.

https://codereview.chromium.org/187573006/diff/1/media/base/video_frame_pool_...
media/base/video_frame_pool_unittest.cc:76: EXPECT_EQ(old_y_data,
new_frame->data(VideoFrame::kYPlane));
On 2014/03/05 16:23:42, Ami Fischman wrote:
> EXPECT_EQ(new_frame->visible_rect(), visible_rect);
> ?

That is done in CreateFrame for all calls to CreateFrame.

[Ami GONE FROM CHROMIUM, 2014-03-05 16:47:14]

My point about subrects is that starting with a VideoFrame whose
visible_rect!=coded_size, there are likely "margins" that are full of
garbage.  It would be unfortunate if these garbage pixels would be
exposable.

Now that I think about it more I realize this will fail to e.g. remove
cropping going from cropped 360p to full 480p, which would be unfortunate.
 I wonder if we need some sort of concept of "valid_rect"...


On Wed, Mar 5, 2014 at 8:39 AM, <perkj@chromium.org> wrote:

> Hej Thanks that was a new record for me in fast review turn arounds I
> think.
> I am not sure I understood all your comments Ami but I tried to address
> them.
>
>
>
> https://codereview.chromium.org/187573006/diff/1/media/base/video_frame.h
> File media/base/video_frame.h (right):
>
> https://codereview.chromium.org/187573006/diff/1/media/
> base/video_frame.h#newcode156
> media/base/video_frame.h:156: const gfx::Rect& visible_rect,
> On 2014/03/05 16:23:42, Ami Fischman wrote:
>
>> the impl makes me think the doco for this function wants to include:
>> "|visible_rect| must be a sub-rect of frame->visible_rect()."
>>
>
>  Then the impl should actually enforce that (DCHECK).
>>
>
> sub size of of codec_size() right?
>
> I DCHECK(IsValidConfig) which seem to validate that and more.
>
>
> https://codereview.chromium.org/187573006/diff/1/media/
> base/video_frame_pool.cc
> File media/base/video_frame_pool.cc (right):
>
> https://codereview.chromium.org/187573006/diff/1/media/
> base/video_frame_pool.cc#newcode75
> media/base/video_frame_pool.cc:75: pool_frame->natural_size() ==
> natural_size) {
> On 2014/03/05 16:23:42, Ami Fischman wrote:
>
>> Should add a subrect test.
>>
>
> ? VideoFramePoolUnitTest::FrameReuseWithDifferentVisibleRect or did I
> missunderstand?
>
>
> https://codereview.chromium.org/187573006/diff/1/media/
> base/video_frame_pool_unittest.cc
> File media/base/video_frame_pool_unittest.cc (right):
>
> https://codereview.chromium.org/187573006/diff/1/media/
> base/video_frame_pool_unittest.cc#newcode14
> media/base/video_frame_pool_unittest.cc:14:
>
> On 2014/03/05 16:17:37, tommi wrote:
>
>> nit: one empty line should be enough
>>
>
> Done.
>
>
> https://codereview.chromium.org/187573006/diff/1/media/
> base/video_frame_pool_unittest.cc#newcode76
> media/base/video_frame_pool_unittest.cc:76: EXPECT_EQ(old_y_data,
> new_frame->data(VideoFrame::kYPlane));
> On 2014/03/05 16:23:42, Ami Fischman wrote:
>
>> EXPECT_EQ(new_frame->visible_rect(), visible_rect);
>> ?
>>
>
> That is done in CreateFrame for all calls to CreateFrame.
>
> https://codereview.chromium.org/187573006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[perkj_chrome, 2014-03-05 17:26:23]

Ok So you are saying its not allowed to render the whole coded size? Cant
that be up to the user of wrapframe to decide ?

I can change as you first suggested so that wrapframe only can wrap
subrects of visible_rect and the same for framepool.

I dont really need the framepool, I just saw that is the only current usage
of wrapframe.

(Written on the subway)
Den 5 mar 2014 17:47 skrev "Ami Fischman" <fischman@chromium.org>:

> My point about subrects is that starting with a VideoFrame whose
> visible_rect!=coded_size, there are likely "margins" that are full of
> garbage.  It would be unfortunate if these garbage pixels would be
> exposable.
>
> Now that I think about it more I realize this will fail to e.g. remove
> cropping going from cropped 360p to full 480p, which would be unfortunate.
>  I wonder if we need some sort of concept of "valid_rect"...
>
>
> On Wed, Mar 5, 2014 at 8:39 AM, <perkj@chromium.org> wrote:
>
>> Hej Thanks that was a new record for me in fast review turn arounds I
>> think.
>> I am not sure I understood all your comments Ami but I tried to address
>> them.
>>
>>
>>
>> https://codereview.chromium.org/187573006/diff/1/media/base/video_frame.h
>> File media/base/video_frame.h (right):
>>
>> https://codereview.chromium.org/187573006/diff/1/media/
>> base/video_frame.h#newcode156
>> media/base/video_frame.h:156: const gfx::Rect& visible_rect,
>> On 2014/03/05 16:23:42, Ami Fischman wrote:
>>
>>> the impl makes me think the doco for this function wants to include:
>>> "|visible_rect| must be a sub-rect of frame->visible_rect()."
>>>
>>
>>  Then the impl should actually enforce that (DCHECK).
>>>
>>
>> sub size of of codec_size() right?
>>
>> I DCHECK(IsValidConfig) which seem to validate that and more.
>>
>>
>> https://codereview.chromium.org/187573006/diff/1/media/
>> base/video_frame_pool.cc
>> File media/base/video_frame_pool.cc (right):
>>
>> https://codereview.chromium.org/187573006/diff/1/media/
>> base/video_frame_pool.cc#newcode75
>> media/base/video_frame_pool.cc:75: pool_frame->natural_size() ==
>> natural_size) {
>> On 2014/03/05 16:23:42, Ami Fischman wrote:
>>
>>> Should add a subrect test.
>>>
>>
>> ? VideoFramePoolUnitTest::FrameReuseWithDifferentVisibleRect or did I
>> missunderstand?
>>
>>
>> https://codereview.chromium.org/187573006/diff/1/media/
>> base/video_frame_pool_unittest.cc
>> File media/base/video_frame_pool_unittest.cc (right):
>>
>> https://codereview.chromium.org/187573006/diff/1/media/
>> base/video_frame_pool_unittest.cc#newcode14
>> media/base/video_frame_pool_unittest.cc:14:
>>
>> On 2014/03/05 16:17:37, tommi wrote:
>>
>>> nit: one empty line should be enough
>>>
>>
>> Done.
>>
>>
>> https://codereview.chromium.org/187573006/diff/1/media/
>> base/video_frame_pool_unittest.cc#newcode76
>> media/base/video_frame_pool_unittest.cc:76: EXPECT_EQ(old_y_data,
>> new_frame->data(VideoFrame::kYPlane));
>> On 2014/03/05 16:23:42, Ami Fischman wrote:
>>
>>> EXPECT_EQ(new_frame->visible_rect(), visible_rect);
>>> ?
>>>
>>
>> That is done in CreateFrame for all calls to CreateFrame.
>>
>> https://codereview.chromium.org/187573006/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ami GONE FROM CHROMIUM, 2014-03-05 17:46:47]

>
> Ok So you are saying its not allowed to render the whole coded size? Cant
> that be up to the user of wrapframe to decide ?
>
If you're handed a VideoFrame you might not know where it came from.  The
way you're supposed to know what's safe to render is vislble_rect.  If v_r
contains lies, you'll be sad :)

> I can change as you first suggested so that wrapframe only can wrap
> subrects of visible_rect and the same for framepool.
>
I don't think this is just a framepool problem.  I think it's a VideoFrame
problem.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[perkj_chrome, 2014-03-05 19:52:19]

Hej
How about one problem at the time. If we need to allow wrapping a video frame in
multiple levels with smaller and larger view_rects we can do that later? 

I uploaded a version that don't allow wrapping frames with view_rects outside
the original view rect.
I reverted all changes to the frame pool except the necessary to compile. 

I added a simple unittest for WrapVideoFrame.

[Ami GONE FROM CHROMIUM, 2014-03-05 20:05:12]

LGTM

https://codereview.chromium.org/187573006/diff/30002/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://codereview.chromium.org/187573006/diff/30002/media/base/video_frame.c...
media/base/video_frame.cc:188: visible_rect.bottom() <=
frame->visible_rect().bottom());
aka DCHECK(frame->visible_rect().Contains(visible_rect));

[perkj_chrome, 2014-03-06 07:40:14]

https://codereview.chromium.org/187573006/diff/30002/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://codereview.chromium.org/187573006/diff/30002/media/base/video_frame.c...
media/base/video_frame.cc:188: visible_rect.bottom() <=
frame->visible_rect().bottom());
On 2014/03/05 20:05:13, Ami Fischman wrote:
> aka DCHECK(frame->visible_rect().Contains(visible_rect));
thanks.

[perkj_chrome, 2014-03-06 14:51:57]

Committed patchset #4 manually as r255353 (presubmit successful).