Closed Bug 1275441 Opened 6 years ago Closed 6 years ago

Use libyuv for scaling YUV color conversion

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
relnote-firefox --- 51+
firefox51 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 12 obsolete files)

+++ This bug was initially created as a clone of Bug #1256475 +++

Bug 1256475 is for non scaling case. This bug is for scaling conversion case.

It would be good to see how the performance of libyuv compares to gfx/ycbcr
No longer depends on: 1273417
Assignee: nobody → sotaro.ikeda.g
Attached patch wip patch (obsolete) — Splinter Review
Attached patch wip patch (obsolete) — Splinter Review
Attachment #8759169 - Attachment is obsolete: true
See Also: → 1254874
Attached patch wip patch (obsolete) — Splinter Review
Attachment #8759170 - Attachment is obsolete: true
Compared fullscreen youtube playback performance between "master + bug 1256475" and "master + bug 1256475 + bug 1254010 + Bug 1275441(this bug)" by using the following video on my laptop.

 - https://www.youtube.com/watch?v=iNJdPyoqt8U


- "master + bug 1256475 + bug 1254010 + Bug 1275441(this bug)"
  + 2160p(4K): 12 - 30 fps (fps was unstable)
  + 1440p    : 23 fps
  + 1080p    : 30 fps
  + 720p     : 30 fps
  + 480p     : 30 fps

- "master + bug 1256475"
  + 2160p(4K): 6 - 15 fps (fps was unstable)
  + 1440p    : 13 fps
  + 1080p    : 20 fps
  + 720p     : 27 fps
  + 480p     : 30 fps
(In reply to Sotaro Ikeda [:sotaro] from comment #4)
> - "master + bug 1256475"

The maser code enables ssse3 scaler(without Bug 1276923).
(In reply to Sotaro Ikeda [:sotaro] from comment #4)
> Compared fullscreen youtube playback performance between "master + bug
> 1256475" and "master + bug 1256475 + bug 1254010 + Bug 1275441(this bug)" by
> using the following video on my laptop.
> 
>  - https://www.youtube.com/watch?v=iNJdPyoqt8U

youtube video fps was 30. Then 30fps was max.
(In reply to Sotaro Ikeda [:sotaro] from comment #4)
> Compared fullscreen youtube playback performance between "master + bug
> 1256475" and "master + bug 1256475 + bug 1254010 + Bug 1275441(this bug)" by
> using the following video on my laptop.
> 
>  - https://www.youtube.com/watch?v=iNJdPyoqt8U
> 
> 
> - "master + bug 1256475 + bug 1254010 + Bug 1275441(this bug)"
>   + 2160p(4K): 12 - 30 fps (fps was unstable)
>   + 1440p    : 23 fps
>   + 1080p    : 30 fps
>   + 720p     : 30 fps
>   + 480p     : 30 fps
> 
> - "master + bug 1256475"
>   + 2160p(4K): 6 - 15 fps (fps was unstable)
>   + 1440p    : 13 fps
>   + 1080p    : 20 fps
>   + 720p     : 27 fps
>   + 480p     : 30 fps

I think for this comparison to be fair we need to be recycling the buffer that we yuv convert into. Right now the we pay for the cost of allocating the buffer and faulting in all of the zero'd pages. Do you think you can hack up a patch to avoid the reallocation and then redo the comparision?
I could create a temporal patch just to avoid the allocation.
Attached patch wip patch (obsolete) — Splinter Review
Move scale yuv conversion code under ycbcr.
Attachment #8759497 - Attachment is obsolete: true
Compared fullscreen youtube playback performance between "master + attachment 8761916 [details] [diff] [review]" and "master + bug 1254010 + attachment 8761895 [details] [diff] [review]" by using the following video on my laptop.

 - https://www.youtube.com/watch?v=iNJdPyoqt8U (30fps)

- "master + bug 1254010 + attachment 8761895 [details] [diff] [review]" (scaling and conversion once if possible)
  + 2160p(4K): 12 - 30 fps (fps was unstable)
  + 1440p    : 23 fps
  + 1080p    : 30 fps
  + 720p     : 30 fps
  + 480p     : 30 fps

- "master + attachment 8761916 [details] [diff] [review]" (with recycle buffer)
  + 2160p(4K): 8 - 23 fps (fps was unstable)
  + 1440p    : 16 fps
  + 1080p    : 22 fps
  + 720p     : 28 fps
  + 480p     : 30 fps

- "master"
  + 2160p(4K): 6 - 15 fps (fps was unstable)
  + 1440p    : 13 fps
  + 1080p    : 20 fps
  + 720p     : 27 fps
  + 480p     : 30 fps

With buffer recycling performance becomes a bit better than default master.
Attached patch wip patch (obsolete) — Splinter Review
Code clean up.
Attachment #8761895 - Attachment is obsolete: true
Attached patch wip patch - Add YUVToARGBScale() (obsolete) — Splinter Review
Attachment #8764854 - Attachment is obsolete: true
Attached patch wip patch - Add YUVToARGBScale() (obsolete) — Splinter Review
Add I444 and I422 support.
Attachment #8765389 - Attachment is obsolete: true
Depends on: 1282711
Attached patch wip patch - Add YUVToARGBScale() (obsolete) — Splinter Review
Fix some bugs.
Attachment #8765795 - Attachment is obsolete: true
Attachment #8761916 - Attachment is obsolete: true
Depends on: 1284803
No longer depends on: 1282711
Attached patch patch - Add YUVToARGBScale() (obsolete) — Splinter Review
Update based on Bug 1284803.
Attachment #8767059 - Attachment is obsolete: true
Attached patch patch - Add YUVToARGBScale() (obsolete) — Splinter Review
Attachment #8775023 - Attachment is obsolete: true
Attached patch patch - Add YUVToARGBScale() (obsolete) — Splinter Review
Fix warnings.
Attachment #8775030 - Attachment is obsolete: true
Blocks: 1254010
Attachment #8775047 - Flags: review?(jmuizelaar)
Comment on attachment 8775047 [details] [diff] [review]
patch - Add YUVToARGBScale()

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

Can you describe the differences between this and scale_argb.cc?
libyuv already has ScaleYUVToARGBBilinearUp(). I implemented the patch based on it.
  https://dxr.mozilla.org/mozilla-central/source/media/libyuv/source/scale_argb.cc#426

At first, I implemented ScaleYUVToARGBBilinearUp() by modidying the above libyuv's one. Then all another functions were implemented similarly.

Function relationship between yuv_convert.cpp abd scale_argb.cc are like the followings
  - ScaleYUVToARGBDown2()      <-- ScaleARGBDown2()
  - ScaleYUVToARGBDownEven()   <-- ScaleARGBDownEven()
  - ScaleYUVToARGBBilinearDown() <-- ScaleARGBBilinearDown()
  - ScaleYUVToARGBBilinearUp() <-- ScaleARGBBilinearUp() and ScaleYUVToARGBBilinearUp() in libyuv
  - ScaleYUVToARGBSimple()     <-- ScaleARGBSimple()
  - ScaleYUVToARGB()           <-- ScaleARGB() // Removed some function calls for simplicity.
  - YUVToARGBScale()           <-- ARGBScale()

I tried to keep callings and selections of InterpolateRow() and ScaleARGBFilterCols() as same as possible.

The followings changes were done to each scaling functions.

 -[1] Allocate YUV conversion buffer and use it as source buffer of scaling.
      Its usage is borrowed from the above libyuv's ScaleYUVToARGBBilinearUp().
 -[2] Conversion from YUV to RGB was abstracted as YUVBuferIter.
      It is for handling multiple yuv color formats.
 -[3] Modified scaling functions as to handle YUV conversion buffer and use YUVBuferIter.
 -[4] Color conversion function selections in YUVBuferIter were borrowed from I444ToARGBMatrix(), I422ToARGBMatrix() and I420ToARGBMatrix() 
      https://dxr.mozilla.org/mozilla-central/source/media/libyuv/source/convert_argb.cc#377
      https://dxr.mozilla.org/mozilla-central/source/media/libyuv/source/convert_argb.cc#209
      https://dxr.mozilla.org/mozilla-central/source/media/libyuv/source/convert_argb.cc#49

Is the description OK for you?
Comment on attachment 8775047 [details] [diff] [review]
patch - Add YUVToARGBScale()

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

It's probably worth including the description that you just wrote.
Attachment #8775047 - Flags: review?(jmuizelaar) → review+
Apply the comment.
Attachment #8775047 - Attachment is obsolete: true
Attachment #8778030 - Flags: review+
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f67a96475ab1
Use libyuv for scaling YUV color conversion r=jrmuizel
https://hg.mozilla.org/mozilla-central/rev/f67a96475ab1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Release Note Request (optional, but appreciated)
[Why is this notable]: 4K videos are playable at high resolution without gpu acceleration.
[Suggested wording]: Further improvements to unaccelerated video performance
relnote-firefox: --- → ?
Added to Fx51 (Aurora) release notes.
In the release notes of 51 with "Improved video performance for users without GPU acceleration:Less CPU Usage & Better full screen experience" wording.
You need to log in before you can comment on or make changes to this bug.