583138 - Update to latest Chromium YCbCr conversion and scaling code

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[cajbir (:cajbir)]

Bug 577843 uses Chromium's YCbCr scaling code to scale videos during RGB color conversion. The latest Chromium source has changed quite a bit and we're using an older version of the scaling and conversion code. None of the patches in gfx/ycbcr apply to the latest Chromium source anymore.

We should update to the latest Chromium code and test to see if there are speed or quality improvements in the scaling code.

Comment 1

[cajbir (:cajbir)]

Bug 577843 degrades playback quality when scaled pretty drastically. If we're going to take that bug in FF 4 we need to take this one as it provides a better quality scaling method. If we're not going to take this one, we shouldn't take bug 577843.

Comment 2

[wencj]

This is only for video, right? Any chances to borrow the code for image scaling too? The Flying Images demo for example is about 3 times faster in Chromium than in Minefield...

Comment 3

[cajbir (:cajbir)]

Attachment: Update to latest Chromium YCbCr code

Updates to latest Chromium code. Re-applies all our existing patches on top of this. Awaiting results of try server and non-libxul builds before asking for review.

Comment 4

[cajbir (:cajbir)]

Comment on attachment 485967 [details] [diff] [review]
Update to latest Chromium YCbCr code

Build errors on OS X 32 and 64 bit. Testing a fix on tryserver and will upload a new patch if/when that works.

Comment 5

[cajbir (:cajbir)]

Attachment: Update to latest Chromium YCbCr code

Builds and passes on tryserver and non-libxul configurations. Let me know if you want this chopped up somehow to make review easier.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I see you've collapsed all our patches into one patch. Is there a reason for that? Wouldn't it be easier to maintain --- and to upstream our patches --- if we still had separate patches at least for build integration, picture region, C fallback, and 4:4:4?

Comment 7

[cajbir (:cajbir)]

It was easier to maintain one patch than multiple patches that have to all be applied in a specific order. When a change has to made to one patch in the middle then all the later patches have to be reapplied and fixed up. The chromium code changed so much from the previous version that I ended up manually adding the changes in from the existing patches - it wasn't possible to apply them at all.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Are we going to upstream stuff? Surely we need separate patches for that?

Comment 9

[cajbir (:cajbir)]

My plan for upstreaming the stuff that I've done is to manually apply the changes to Chromium's implementation, generate the patch and submit that to them. When we next pull from Chromium we'll pick that up and regenerate our combined patch, which will no longer contain our fixes.

Due to the changes made to have runtime CPU detection and C fallback the patches for our tree and Chromiums tree would need to be very different.

It is too difficult to manage multiple separate patches all dependent on each other in our tree. When someone submits a fix we have to either modify one of the existing patches, and then manually rebase all the dependent patches on top of it, or add a new orphaned patch for just that change. The latter results in many small patches which become hard to apply or associate with the original change.

For now I'd rather keep one combined patch, and any new orphaned patches as people provide fixes if needed. Periodically refresh against Chromium and start with one patch again. This patch gets smaller as things get upstreamed.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #9)
> My plan for upstreaming the stuff that I've done is to manually apply the
> changes to Chromium's implementation, generate the patch and submit that to
> them. When we next pull from Chromium we'll pick that up and regenerate our
> combined patch, which will no longer contain our fixes.
> 
> Due to the changes made to have runtime CPU detection and C fallback the
> patches for our tree and Chromiums tree would need to be very different.

Don't we want to upstream runtime CPU detection and C fallback?

> For now I'd rather keep one combined patch, and any new orphaned patches as
> people provide fixes if needed. Periodically refresh against Chromium and start
> with one patch again. This patch gets smaller as things get upstreamed.

OK.

Comment 11

[Benjamin Smedberg]

What's the status of this? It's blocking 604307 which is important to quite a few people running Linux.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It's ready to land.

Comment 13

[cajbir (:cajbir)]

Assuming the tree is ok for landing I'll be landing it this afternoon. The patch as is won't land as it has a slight problem. It contains the .patch file for the chromium changes but that patch isn't actually applied. I'm fixing this, waiting for a build and will push when I can.

Comment 14

[cajbir (:cajbir)]

Attachment: Update to latest Chromium YCbCr code

Fixes the issue where the .patch file wasn't applied. Carried r+ forward.

Comment 15

[cajbir (:cajbir)]

http://hg.mozilla.org/mozilla-central/rev/64901a1fcf93

Comment 16

[Justin Lebar (not reading bugmail)]

> #if defined(MOZILLA_COMPILE_WITH_SSE2)
> static void FilterRows(uint8* ybuf, const uint8* y0_ptr, const uint8* y1_ptr,
>                        int source_width, int source_y_fraction) {

I don't have an old CPU to test on, but I think this will try and run SSE2 code on any Windows machine, even if it doesn't support SSE2.

MOZILLA_COMPILE_WITH_SSE2 is always defined on Windows, and I don't see any cpuid guards around the call to FilterRows.  In fact, the unvectorized implementation isn't even compiled when MOZILLA_COMPILE_WITH_SSE2 is defined.

See also bug 585708.

Comment 17

[Justin Lebar (not reading bugmail)]

Filed bug 616778 regarding comment 16.