1256475 - Use libyuv for non scaling YUV color conversion

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

It would be good to see how the performance of libyuv compares to gfx/ycbcr

Comment 1

[Lee Salzman [:lsalzman]]

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.

Comment 2

[Lee Salzman [:lsalzman]]

(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.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

See also bug 791941, it may be a duplicate.

Comment 4

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Update DataSourceSurfaceFromYCbCrDescriptor

Comment 5

[Sotaro Ikeda [:sotaro]]

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

Moved to Bug 1273417.

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Just roughly connect libyuv for non scaling conversion to RGB32.

Comment 7

[Sotaro Ikeda [:sotaro]]

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

Comment 8

[Sotaro Ikeda [:sotaro]]

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.

Comment 9

[Sotaro Ikeda [:sotaro]]

(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

Comment 10

[Sotaro Ikeda [:sotaro]]

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".

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Use libyuv for non scaling YUV color conversion

Comment 12

[Sotaro Ikeda [:sotaro]]

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.

Comment 13

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 14

[Sotaro Ikeda [:sotaro]]

Some functions in libyuv have sse2, but Yuv to RGB color conversions do not have sse2.

Comment 15

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6052842509e2

Comment 16

[Sotaro Ikeda [:sotaro]]

reftest failures on linux :(

Comment 17

[Sotaro Ikeda [:sotaro]]

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

Comment 18

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Update YuvPixel calculation

It make YUV to RGB conversion precision a bit better.

Comment 19

[Sotaro Ikeda [:sotaro]]

(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

Comment 20

[Sotaro Ikeda [:sotaro]]

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)) */

Comment 21

[Sotaro Ikeda [:sotaro]]

(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.

Comment 22

[Sotaro Ikeda [:sotaro]]

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.

Comment 23

[Sotaro Ikeda [:sotaro]]

Precise color conversion is necessary only during some reftests. It seems better to fallback to old function only during the tests.

Comment 24

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Use old function on some reftests

Comment 25

[Sotaro Ikeda [:sotaro]]

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.

Comment 26

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Use old function on some reftests

Fixed warning.

Comment 27

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=11d890bc85b9

Comment 28

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 29

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Use old function on some reftests

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c88f2cb8e4fa
Use libyuv for non scaling YUV color conversion r=jrmuizel

Comment 31

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d8c954e520e4 - looks like Android needs something for that to land, https://treeherder.mozilla.org/logviewer.html#?job_id=29645198&repo=mozilla-inbound

Comment 32

[Sotaro Ikeda [:sotaro]]

Ah, bug 1141979 was landed since tryserver check in comment 27.

Comment 33

[Sotaro Ikeda [:sotaro]]

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

Comment 34

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Use old function in test_imagebitmap_extensions.html

Comment 35

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=63bd79890702

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d21ad903c7a
Use libyuv for non scaling YUV color conversion r=jrmuizel

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6d21ad903c7a