641014 - NEON Y'CbCr to RGB conversion broken for everything but 4:2:0 with no picture offset

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Timothy B. Terriberry (:derf)]

This function makes no attempt to handle any other cases, but we call it for those cases anyway. The asm in yv12_to_rgb565_neon should actually work fine for 4:2:2 and with a picture offset as well (which is passed in to ConvertYCbCrToRGB565 but ignored). 4:4:4 support will require either an asm rewrite or bug 640964.

Steps to reproduce (offsets):
1. Use a device with NEON (N900, non-Tegra2 Android)
2. Visit http://v2v.cc/~j/theora_testsuite/offset_test.ogv

Expected results: http://v2v.cc/~j/theora_testsuite/offset_test.pass.png
Actual results: http://v2v.cc/~j/theora_testsuite/offset_test.fail.png (or at least, the upper-left 512x512 portion of it)

Steps to reproduce (4:2:2 content):
1. Use a device with NEON (N900, non-Tegra2 Android)
2. Visit http://v2v.cc/~j/theora_testsuite/mobile_itu601_i_422.ogg

Expected results: normal colors (e.g., the yellow in the chicks on the wallpaper should stay within their outline)
Actual results: colors spread out to twice their normal height (the yellow in the chicks on the wallpaper bleeds into the area below them, as does everything else).

Steps to reproduce (4:4:4 content):
1. Use a device with NEON (N900, non-Tegra2 Android)
2. Visit http://v2v.cc/~j/theora_testsuite/ducks_take_off_444_720p25.ogg

Expected results: the ducks should not be blue.
Actual results: the ducks are blue (and there are duck-shaped shadows on the water to their lower-right)

Comment 1

[Timothy B. Terriberry (:derf)]

Attachment: Proposed fix

The underlying NEON function can actually support a picture offset (even an odd one), and can also be made to work for 4:2:2 content. This patch adds support for that to the code that calls it, and adds a simple fallback C implementation for 4:4:4 content.

It also adds an IsConvertYCbCrToRGB565Fast() in place of the previous have_ycbcr_to_rgb565() (which would now always be true whenever HAVE_YCBCR_TO_RGB565 is defined). Instead of leaving stub functions which don't do what is intended when HAVE_YCBCR_TO_RGB565 is not defined, these functions are removed entirely, ensuring compile/link errors if someone attempts to use them when they aren't there.

The new functions have been placed in the mozilla::gfx namespace, and the old functions moved there as well, which should also fix the compilation issue reported against the patch in bug 637961.

Additionally, it moves ConvertYCbCrToRGB565 out of the Chromium files and into the (already existing) Mozilla-specific ycbcr_to_rgb565.cpp and ycbcr_to_rgb565.h files. This code already wasn't being tracked properly in convert.patch (these new files were not being added to it), and it isn't really related to the Chromium code, except in function; the implementations are independent.

This patch applies on top of the patches from bug 639415 and bug 583958.

Comment 2

[Timothy B. Terriberry (:derf)]

Attachment: Proposed fix (version 2)

The prior patch didn't quite get rid of all the ARM-specific stuff in convert.patch (I had written a script to verify that it did, but my script had an error). This version of the patch corrects that deficiency. Carrying forward r=doublec.

Comment 3

[Timothy B. Terriberry (:derf)]

Attachment: Proposed fix (version 3)

The previous version of this patch accidentally left out an "else" after the supports_neon() check in the MOZILLA_MAY_SUPPORT_NEON block of ConvertYCbCrToRGB565(), so it was actually running the conversion twice when NEON was supported (once using the NEON code, and then again using plain C). I noticed when working back towards the full patch in bug 634557, which correctly included the else.

Comment 4

[Timothy B. Terriberry (:derf)]

Comment on attachment 520047 [details] [diff] [review]
Proposed fix (version 3)

Continuing to carry forward r=doublec.

Comment 5

[Timothy B. Terriberry (:derf)]

(In reply to comment #1)
> The new functions have been placed in the mozilla::gfx namespace, and the old
> functions moved there as well, which should also fix the compilation issue
> reported against the patch in bug 637961.

Confirmed in bug 634557#c44.

Comment 6

[cajbir (:cajbir)]

http://hg.mozilla.org/mozilla-central/rev/acf9030b80b4

Comment 7

[Andreea Pod]

Tried to verify it on N900 but the links are not working, I get "404 Not Found" error. Can you provide another links so I can verify the bug?

Comment 8

[Timothy B. Terriberry (:derf)]

They all work for me, but I've put up mirrors at:
http://media.xiph.org/video/theora_testsuite/offset_test.ogv
http://media.xiph.org/video/theora_testsuite/offset_test.pass.png
http://media.xiph.org/video/theora_testsuite/offset_test.fail.png
http://media.xiph.org/video/theora_testsuite/mobile_itu601_i_422.ogg
http://media.xiph.org/video/theora_testsuite/ducks_take_off_444_720p25.ogg

Let me know if those work any better.

Comment 9

[Andreea Pod]

Yes, these links are working and the problems are fixed. 

Verified on build: Mozilla /5.0 (Maemo;Linux armv7l;rv:2.2a1pre) Gecko/20110411 Firefox/4.2a1pre Fennec /4.1a1pre
Device: Nokia N900