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

VERIFIED FIXED in mozilla5

Status

()

defect
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: derf, Assigned: derf)

Tracking

Trunk
mozilla5
ARM
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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)
Assignee

Updated

8 years ago
Summary: Y'CbCr to RGB conversion broken for everything but 4:2:0 with no picture offset → NEON Y'CbCr to RGB conversion broken for everything but 4:2:0 with no picture offset
Assignee

Updated

8 years ago
Depends on: 583958
Assignee

Comment 1

8 years ago
Posted patch Proposed fix (obsolete) — Splinter Review
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.
Assignee: nobody → tterribe
Status: NEW → ASSIGNED
Attachment #519580 - Flags: review?(chris.double)

Updated

8 years ago
Attachment #519580 - Flags: review?(chris.double) → review+
Assignee

Comment 2

8 years ago
Posted patch Proposed fix (version 2) (obsolete) — Splinter Review
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.
Attachment #519580 - Attachment is obsolete: true
Attachment #519946 - Flags: review+
Assignee

Comment 3

8 years ago
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.
Attachment #519946 - Attachment is obsolete: true
Attachment #520047 - Flags: review+
Assignee

Comment 4

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

Continuing to carry forward r=doublec.
Assignee

Comment 5

8 years ago
(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

8 years ago
http://hg.mozilla.org/mozilla-central/rev/acf9030b80b4
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.2

Comment 7

8 years ago
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 9

8 years ago
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.