Use libyuv for non scaling YUV color conversion

RESOLVED FIXED in Firefox 50

Status

()

Core
Graphics
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jrmuizel, Assigned: sotaro)

Tracking

(Blocks: 1 bug)

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(3 attachments, 5 obsolete attachments)

It would be good to see how the performance of libyuv compares to gfx/ycbcr
Assignee: nobody → ethlin
Blocks: 1253062
One thing that is worth noting is libyuv does not have a combined conversion and scaling routine - the upstream implementation simply converts first, then upscales the RGB, so no better than the status quo.  Whereas ycbcr does have a combined version, as well as clipped scaling in upstream. Just a FYI.
(In reply to Lee Salzman [:lsalzman] from comment #1)
> One thing that is worth noting is libyuv does not have a combined conversion
> and scaling routine - the upstream implementation simply converts first,
> then upscales the RGB, so no better than the status quo.  Whereas ycbcr does
> have a combined version, as well as clipped scaling in upstream. Just a FYI.

Actually, just looked at libyuv master and maybe the situation has actually improved a bit, hmm.
Assignee: ethlin → sotaro.ikeda.g
See also bug 791941, it may be a duplicate.
(Assignee)

Updated

2 years ago
See Also: → bug 1254872
(Assignee)

Updated

2 years ago
See Also: → bug 791941
(Assignee)

Comment 4

2 years ago
Created attachment 8753195 [details] [diff] [review]
patch part 1 - Update DataSourceSurfaceFromYCbCrDescriptor
(Assignee)

Updated

2 years ago
Depends on: 1273417
(Assignee)

Comment 5

2 years ago
Comment on attachment 8753195 [details] [diff] [review]
patch part 1 - Update DataSourceSurfaceFromYCbCrDescriptor

Moved to Bug 1273417.
Attachment #8753195 - Attachment is obsolete: true
(Assignee)

Comment 6

2 years ago
Created attachment 8753669 [details] [diff] [review]
wip patch

Just roughly connect libyuv for non scaling conversion to RGB32.
(Assignee)

Comment 7

2 years ago
Compare performance with/without attachment 8753669 [details] [diff] [review] on my pc.

Uses the following video with 1080p.
   https://www.youtube.com/watch?v=e-ORhEE9VVg
Measure the conversion time in DataSourceSurfaceFromYCbCrDescriptor()
   https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ImageDataSerializer.cpp#175

Result
 - Without the patch: about 8ms
 - With the patch: about 5ms

With the patch, I422ToARGBRow_AVX2() was used for the video color conversion on my pc.
   https://dxr.mozilla.org/mozilla-central/source/media/libyuv/source/row_win.cc#2081
(Assignee)

Comment 8

2 years ago
From the following, it seems better just to use libyuv for non scaling color convert from YUV to RGB32 in this bug.

- One step ScaleYUVToRGB32 is not implemented in libyuv
    + Two step scaling by using libyuv might be faster than one step 
      chromium ycbcr code(Bug 791941 Comment 6).
- Color convert to RGB565 is not well supported in libyuv
    + Even on gecko, convert to RGB565 is not common now.
(Assignee)

Comment 9

2 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #8)
> From the following, it seems better just to use libyuv for non scaling color
> convert from YUV to RGB32 in this bug.
> 
> - One step ScaleYUVToRGB32 is not implemented in libyuv
>     + Two step scaling by using libyuv might be faster than one step 
>       chromium ycbcr code(Bug 791941 Comment 6).

libyuv seems to have an implementation of Two step scaling as ScaleYUVToARGBBilinearUp(), though the function is not exposed as public function.
  https://dxr.mozilla.org/mozilla-central/source/media/libyuv/source/scale_argb.cc#398
(Assignee)

Comment 10

2 years ago
Compared color conversion performance by using the following 4k video.
  https://www.youtube.com/watch?v=M9GHkHJiFuQ

It is because, libyuv::I420ToARGB() support only SSSE3 and AVX2 as simd implementation for intel cpu. ConvertYCbCrToRGB32() requests only mmx and sse.

Measured on my pc[Intel Core i7-6700k(4GHz)].
 - libyuv::I420ToARGB() AVX2    : 8ms
 - libyuv::I420ToARGB() ssse3   : 9ms
 - libyuv::I420ToARGB() c       : 54msms
 - ConvertYCbCrToRGB32() mmx/sse:12ms
 - ConvertYCbCrToRGB32() c      :120ms

From the above, if chip does not support ssse3, libyuv::I420ToARGB() fallback to c and it degrade performance compared to "ConvertYCbCrToRGB32() mmx/sse".
(Assignee)

Comment 11

2 years ago
Created attachment 8755724 [details] [diff] [review]
patch part 1 - Use libyuv for non scaling YUV color conversion
Attachment #8753669 - Attachment is obsolete: true
(Assignee)

Comment 12

2 years ago
Comment on attachment 8755724 [details] [diff] [review]
patch part 1 - Use libyuv for non scaling YUV color conversion

:jrmuizel, can you feedback to the patch? It uses libyuv on non-scaling YUV color conversion. And it continue to use old function when intel cpu supports mmx&sse but not support ssse3, since libyuv::I420ToARGB() uses only simd of SSSE3 and AVX2.
Attachment #8755724 - Flags: feedback?(jmuizelaar)
(Assignee)

Updated

2 years ago
Summary: Port video to use libyuv instead of gfx/ycbcr → Use libyuv for non scaling YUV color conversion
(Assignee)

Updated

2 years ago
Blocks: 1275441
Comment on attachment 8755724 [details] [diff] [review]
patch part 1 - Use libyuv for non scaling YUV color conversion

Review of attachment 8755724 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I'm surprised libyuv doesn't have an sse2 function.
Attachment #8755724 - Flags: feedback?(jmuizelaar) → feedback+
(Assignee)

Comment 14

2 years ago
Some functions in libyuv have sse2, but Yuv to RGB color conversions do not have sse2.
(Assignee)

Comment 16

2 years ago
reftest failures on linux :(
(Assignee)

Comment 17

2 years ago
Libyuv in gecko seems to use similar calculation to the following. The difference seems to add more conversion differences.

  - "3.1 Removing Floating-Point Computations"
      http://lestourtereaux.free.fr/papers/data/yuvrgb.pdf

YuvPixel() in gecko.
  https://dxr.mozilla.org/mozilla-central/source/media/libyuv/source/row_common.cc#958


By the way, master libyuv seems to cause more difference to get performance.
  https://chromium.googlesource.com/external/libyuv/+/master/source/row_common.cc#1018
(Assignee)

Comment 18

2 years ago
Created attachment 8758147 [details] [diff] [review]
patch - Update YuvPixel calculation

It make YUV to RGB conversion precision a bit better.
(Assignee)

Comment 19

2 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #18)
> Created attachment 8758147 [details] [diff] [review]
> patch - Update YuvPixel calculation
> 
> It make YUV to RGB conversion precision a bit better.

attachment 8758147 [details] [diff] [review] is crated based on http://lestourtereaux.free.fr/papers/data/yuvrgb.pdf
(Assignee)

Comment 20

2 years ago
Comment on attachment 8758147 [details] [diff] [review]
patch - Update YuvPixel calculation

Ah, this patch could not be used. The following cause the problem in simd version. Max of signed byte is 127. 129 could not be used for signed byte.

> -#define UB 127 /* min(63,(int8)(2.018 * 64)) */
> +#define UB 129 /* min(63,(int8)(2.018 * 64 + 0.5)) */
Attachment #8758147 - Attachment is obsolete: true
(Assignee)

Comment 21

2 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #17)
> By the way, master libyuv seems to cause more difference to get performance.
>  
> https://chromium.googlesource.com/external/libyuv/+/master/source/row_common.cc#1018

master libyuv code seems to try color conversion better by using -128 instead of 127. But the color conversion is still not precise enough to pass reftest.
(Assignee)

Comment 22

2 years ago
YCbCr uses table to convert to RGB. But libyuv calculate RGB color to use SIMD more. For the calculation, signed byte and unsigned byte are used as source and signed word is used for calculation. Then when we use libyuv, some color conversion differences seems unavoidable.
(Assignee)

Comment 23

2 years ago
Precise color conversion is necessary only during some reftests. It seems better to fallback to old function only during the tests.
(Assignee)

Comment 24

2 years ago
Created attachment 8758519 [details] [diff] [review]
patch - Use old function on some reftests
(Assignee)

Comment 25

2 years ago
In summary, libyuv conversion from yuv to RGB is not accurate.
- Calculate YUV to RGB to dynamically during the conversion to use optimize for simd.
    + YCbCr uses table
- Signed byte is used for conversion's coefficient. But it requests 129. libyuv cut 129 to 127.
- Only 6 bits are used for a decimal part during the dynamic calculation.
(Assignee)

Comment 26

2 years ago
Created attachment 8758540 [details] [diff] [review]
patch - Use old function on some reftests

Fixed warning.
Attachment #8758519 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8755724 - Flags: review?(jmuizelaar)
(Assignee)

Updated

2 years ago
Attachment #8758540 - Flags: review?(jmuizelaar)
Comment on attachment 8758540 [details] [diff] [review]
patch - Use old function on some reftests

Review of attachment 8758540 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/ycbcr/yuv_convert.cpp
@@ +64,5 @@
>                           YUVType yuv_type) {
>  
> +
> +  // Deprecated function's conversion is accurate.
> +  // libyuv converion is a bit inaccurate to get performance.

It would be good to elaborate this with the information that you wrote about the precision.
Attachment #8758540 - Flags: review?(jmuizelaar) → review+
Attachment #8755724 - Flags: review?(jmuizelaar) → review+
(Assignee)

Updated

2 years ago
Attachment #8755724 - Attachment description: patch - Use libyuv for non scaling YUV color conversion → patch part 1 - Use libyuv for non scaling YUV color conversion
(Assignee)

Comment 29

2 years ago
Created attachment 8759533 [details] [diff] [review]
patch part 2 - Use old function on some reftests
Attachment #8758540 - Attachment is obsolete: true
Attachment #8759533 - Flags: review+

Comment 30

2 years ago
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c88f2cb8e4fa
Use libyuv for non scaling YUV color conversion r=jrmuizel
(Assignee)

Comment 32

2 years ago
Ah, bug 1141979 was landed since tryserver check in comment 27.
(Assignee)

Comment 33

2 years ago
I444ToARGBRow_C() has 2 versions. When arm neon is enabled, the conversion is not accurate. u and v are sub-sampled. It seems to cause the test failure.
 https://dxr.mozilla.org/mozilla-central/source/media/libyuv/source/row_common.cc#970
(Assignee)

Comment 34

2 years ago
Created attachment 8761053 [details] [diff] [review]
patch part 3 - Use old function in test_imagebitmap_extensions.html
(Assignee)

Updated

2 years ago
Attachment #8761053 - Flags: review?(jmuizelaar)
Attachment #8761053 - Flags: review?(jmuizelaar) → review+

Comment 36

2 years ago
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d21ad903c7a
Use libyuv for non scaling YUV color conversion r=jrmuizel

Comment 37

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6d21ad903c7a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.