Closed Bug 1256475 Opened 4 years ago Closed 3 years ago

Use libyuv for non scaling YUV color conversion

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: jrmuizel, Assigned: sotaro)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 5 obsolete files)

It would be good to see how the performance of libyuv compares to gfx/ycbcr
Assignee: nobody → ethlin
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.
See Also: → 1254872
See Also: → 791941
Depends on: 1273417
Comment on attachment 8753195 [details] [diff] [review]
patch part 1 - Update DataSourceSurfaceFromYCbCrDescriptor

Moved to Bug 1273417.
Attachment #8753195 - Attachment is obsolete: true
Attached patch wip patch (obsolete) — Splinter Review
Just roughly connect libyuv for non scaling conversion to RGB32.
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
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.
(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
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 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)
Summary: Port video to use libyuv instead of gfx/ycbcr → Use libyuv for non scaling YUV color conversion
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+
Some functions in libyuv have sse2, but Yuv to RGB color conversions do not have sse2.
reftest failures on linux :(
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
It make YUV to RGB conversion precision a bit better.
(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 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
(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.
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.
Precise color conversion is necessary only during some reftests. It seems better to fallback to old function only during the tests.
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.
Fixed warning.
Attachment #8758519 - Attachment is obsolete: true
Attachment #8755724 - Flags: review?(jmuizelaar)
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+
Attachment #8755724 - Attachment description: patch - Use libyuv for non scaling YUV color conversion → patch part 1 - Use libyuv for non scaling YUV color conversion
Attachment #8758540 - Attachment is obsolete: true
Attachment #8759533 - Flags: review+
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c88f2cb8e4fa
Use libyuv for non scaling YUV color conversion r=jrmuizel
Ah, bug 1141979 was landed since tryserver check in comment 27.
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
Attachment #8761053 - Flags: review?(jmuizelaar)
Attachment #8761053 - Flags: review?(jmuizelaar) → review+
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d21ad903c7a
Use libyuv for non scaling YUV color conversion r=jrmuizel
https://hg.mozilla.org/mozilla-central/rev/6d21ad903c7a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.