media: Handling YV12 odd height/width in vpx_video_decoder

media: Handling YV12 odd height/width in vpx_video_decoder

vpx_video_decoder does not allow odd videos with odd height/width
to playback since it is not valid YV12. But ffmpeg currently
supports it by merely rounding up (and so does the libvpx library).
Changing vpx_video_decoder to behave the same way as ffmpeg and
getting rid of the Checks.

BUG=315817
TEST=media_unittests

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

[scherkus (not reviewing), 2013-11-27 22:31:38]

can we get a test?

I believe I provided one in the bug

[vignesh, 2013-11-27 23:10:32]

On 2013/11/27 22:31:38, scherkus wrote:
> can we get a test?

this code is going away soon (since we are trying to eliminate the copy anyway).
once that happens, we will merely rely on libvpx/ffmpeg's behavior for odd
width/height files. do we need a test for the interim?

> 
> I believe I provided one in the bug

yes, you did. i'm not sure about it's license though. i can generate a
bear-odd.webm from one of the existing files if a test is required.

[scherkus (not reviewing), 2013-12-02 18:37:26]

On 2013/11/27 23:10:32, vignesh wrote:
> On 2013/11/27 22:31:38, scherkus wrote:
> > can we get a test?
> 
> this code is going away soon (since we are trying to eliminate the copy
anyway).
> once that happens, we will merely rely on libvpx/ffmpeg's behavior for odd
> width/height files. do we need a test for the interim?
> 
> > 
> > I believe I provided one in the bug
> 
> yes, you did. i'm not sure about it's license though. i can generate a
> bear-odd.webm from one of the existing files if a test is required.

an odd-dimensioned video sounds like a useful test file to have regardless of
whether a copy is involved or not -- can we get one generated?

[xhwang, 2013-12-02 19:10:17]

On 2013/12/02 18:37:26, scherkus wrote:
> On 2013/11/27 23:10:32, vignesh wrote:
> > On 2013/11/27 22:31:38, scherkus wrote:
> > > can we get a test?
> > 
> > this code is going away soon (since we are trying to eliminate the copy
> anyway).
> > once that happens, we will merely rely on libvpx/ffmpeg's behavior for odd
> > width/height files. do we need a test for the interim?
> > 
> > > 
> > > I believe I provided one in the bug
> > 
> > yes, you did. i'm not sure about it's license though. i can generate a
> > bear-odd.webm from one of the existing files if a test is required.
> 
> an odd-dimensioned video sounds like a useful test file to have regardless of
> whether a copy is involved or not -- can we get one generated?

FWIW, here's one that could trigger this bug: http://a.pomf.se/q5a5h.webm

[vignesh, 2013-12-02 19:31:40]

Have created a VP8A file with odd width and height here:
https://codereview.chromium.org/99533003

Adding a pipeline integration test to test this.

[scherkus (not reviewing), 2013-12-02 20:27:33]

lgtm w/ nit

also as mentioned in https://codereview.chromium.org/99533003 please move the
README change to that CL

https://codereview.chromium.org/92073002/diff/60001/media/filters/vpx_video_d...
File media/filters/vpx_video_decoder.cc (right):

https://codereview.chromium.org/92073002/diff/60001/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:358: (1 + vpx_image->d_h) / 2,
nit: can we swap the order of the variable and the "+1"?

similar with conditions, we try to have variables go first in expressions for
the sake of readability

[vignesh, 2013-12-02 21:07:33]

PTAL

https://codereview.chromium.org/92073002/diff/60001/media/filters/vpx_video_d...
File media/filters/vpx_video_decoder.cc (right):

https://codereview.chromium.org/92073002/diff/60001/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:358: (1 + vpx_image->d_h) / 2,
On 2013/12/02 20:27:33, scherkus wrote:
> nit: can we swap the order of the variable and the "+1"?
> 
> similar with conditions, we try to have variables go first in expressions for
> the sake of readability

Done.

[scherkus (not reviewing), 2013-12-02 21:18:32]

lgtm!

thanks!

[commit-bot: I haz the power, 2013-12-02 21:21:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vigneshv@chromium.org/92073002/40002

[commit-bot: I haz the power, 2013-12-02 23:54:11]

Change committed as 238237