Treat YU12 as an alias for I420. Simplify setting of inv_crop_height.

Treat YU12 as an alias for I420. Simplify setting of inv_crop_height.

BUG=
R=fbarchard@google.com

Committed: https://chromium.googlesource.com/libyuv/libyuv/+/365ed3851c427772580014382b0302bfdb405dd2

[nisse-chromium (ooo August 14), 2016-05-31 09:37:30]

nisse@chromium.org changed reviewers:
+ magjed@chromium.org

[nisse-chromium (ooo August 14), 2016-05-31 09:37:30]

Handle YU12 as an alias for I420 in CanonicalFourCC.

Simplify initialization of inv_crop_height.

BTW, I don't understand that logic. ConvertI420 accepts crop_height < 0 as
input, but the first thing it does is discarding the sign.

And then it sets inv_crop_height using the same sign as the input src_height,
where negative values indicate flipping.

There are few unit tests for ConvertToI420 (there's a bit better coverage of the
underlying functions, I think), and after a quick look I found no tests for
flipping.

Also, for the conversion functions which take a stride as input, I find it a bit
strange to use negative height to indicate vertical flip; it would be natural
with a negative stride (but that's what most functions do in their initial
setup, when setting up the pointers, so maybe negative height can be considered
more user-friendly).
 
The reason I look at this function now is that I'm consider adding validation of
sample_size for all formats, which would make me feel a bit safer in deleting
the cricket::VideoFrame::Validate function in webrtc. If needed validation could
be optional for everything but MJPEG, by letting a zero sample_size indicate
that no validation is wanted. And I guess I ought to file a bug about this.

[magjed_chromium, 2016-05-31 09:48:31]

Description was changed from

==========
Treat YU12 as an alias for I420. Simplify setting of inv_crop_height.

BUG=
==========

to

==========
Treat YU12 as an alias for I420. Simplify setting of inv_crop_height.

BUG=
==========

[magjed_chromium, 2016-05-31 09:48:31]

magjed@chromium.org changed reviewers:
+ fbarchard@chromium.org

[magjed_chromium, 2016-05-31 09:48:37]

https://codereview.chromium.org/2020193002/diff/1/source/convert_to_i420.cc
File source/convert_to_i420.cc (right):

https://codereview.chromium.org/2020193002/diff/1/source/convert_to_i420.cc#n...
source/convert_to_i420.cc:44: int abs_crop_height = (crop_height < 0) ?
-crop_height : crop_height;
Can you make this const as well then?

https://codereview.chromium.org/2020193002/diff/1/source/convert_to_i420.cc#n...
source/convert_to_i420.cc:56: int inv_crop_height = (src_height < 0) ?
-abs_crop_height : abs_crop_height;
ditto: Can you make this variable const?

[nisse-chromium (ooo August 14), 2016-05-31 12:47:31]

https://codereview.chromium.org/2020193002/diff/1/source/convert_to_i420.cc
File source/convert_to_i420.cc (right):

https://codereview.chromium.org/2020193002/diff/1/source/convert_to_i420.cc#n...
source/convert_to_i420.cc:44: int abs_crop_height = (crop_height < 0) ?
-crop_height : crop_height;
On 2016/05/31 09:48:37, magjed_chromium wrote:
> Can you make this const as well then?

Done.

https://codereview.chromium.org/2020193002/diff/1/source/convert_to_i420.cc#n...
source/convert_to_i420.cc:56: int inv_crop_height = (src_height < 0) ?
-abs_crop_height : abs_crop_height;
On 2016/05/31 09:48:37, magjed_chromium wrote:
> ditto: Can you make this variable const?

Done.

[fbarchard1, 2016-05-31 18:26:39]

On 2016/05/31 12:47:31, nisse-chromium wrote:
> https://codereview.chromium.org/2020193002/diff/1/source/convert_to_i420.cc
> File source/convert_to_i420.cc (right):
> 
>
https://codereview.chromium.org/2020193002/diff/1/source/convert_to_i420.cc#n...
> source/convert_to_i420.cc:44: int abs_crop_height = (crop_height < 0) ?
> -crop_height : crop_height;
> On 2016/05/31 09:48:37, magjed_chromium wrote:
> > Can you make this const as well then?
> 
> Done.
> 
>
https://codereview.chromium.org/2020193002/diff/1/source/convert_to_i420.cc#n...
> source/convert_to_i420.cc:56: int inv_crop_height = (src_height < 0) ?
> -abs_crop_height : abs_crop_height;
> On 2016/05/31 09:48:37, magjed_chromium wrote:
> > ditto: Can you make this variable const?
> 
> Done.

best to file a bug and enter a TESTED=

The reason the I420 and YU12 were un-aliased was a bug on linux YU12 for libv4l
we called the 'predator bug' on 'lucid' ubuntu timeframe. 
But should be fine to alias them again; they are the same format.

[fbarchard1, 2016-05-31 18:28:10]

fbarchard@google.com changed reviewers:
+ fbarchard@google.com

[fbarchard1, 2016-05-31 18:28:11]

lgtm with caveat that it will re-introduce the 'predator' bug for v4l.

[fbarchard1, 2016-06-01 00:05:14]

update unittests in video_common_test?
TestCanonicalFourCC tests aliases.
TestFourCC tests non-aliases.

Can simplify conversions that include YU12 and I420 tests.
e.g. ConvertToI420

[magjed_chromium, 2016-06-01 13:32:44]

We still use libv4l on Linux as far as I know. We should test this CL before
landing it so we don't reintroduce the bug.

[nisse-chromium (ooo August 14), 2016-06-02 08:11:51]

On 2016/06/01 13:32:44, magjed_chromium_ooo_june27 wrote:
> We still use libv4l on Linux as far as I know. We should test this CL before
> landing it so we don't reintroduce the bug.

I'm not sure I understand the implications... As far as I understood from
skimming the old discussion, webrtc worked around that bug webrtc by keeping
YU12 (used on linux) and I420 (used on windows) as distinct, in order to give
them different priorities in format selection in the video capturer?

Does that really depend on libyuv's CanonicalFourCC? webrtc uses its own
incarnation of this function, see
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

(I'd like to eliminate this duplication some day; at that point webrtc will
depend on libyuv's version).

[nisse-chromium (ooo August 14), 2016-06-16 10:49:22]

Description was changed from

==========
Treat YU12 as an alias for I420. Simplify setting of inv_crop_height.

BUG=
==========

to

==========
Treat YU12 as an alias for I420. Simplify setting of inv_crop_height.

BUG=
R=fbarchard@google.com

Committed:
https://chromium.googlesource.com/libyuv/libyuv/+/365ed3851c427772580014382b0...
==========

[nisse-chromium (ooo August 14), 2016-06-16 10:49:24]

Committed patchset #3 (id:40001) manually as
365ed3851c427772580014382b0302bfdb405dd2 (presubmit successful).