Update to latest Chromium YCbCr conversion and scaling code

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
3 years ago

People

(Reporter: cajbir, Assigned: cajbir)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

8 years ago
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.
(Assignee)

Updated

8 years ago
Depends on: 577843
(Assignee)

Comment 1

8 years ago
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.
blocking2.0: --- → ?
blocking2.0: ? → final+

Comment 2

8 years ago
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...
Assignee: nobody → chris.double
(Assignee)

Updated

8 years ago
Blocks: 576764
Blocks: 604307
(Assignee)

Comment 3

8 years ago
Created attachment 485967 [details] [diff] [review]
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.
(Assignee)

Comment 4

8 years ago
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.
Attachment #485967 - Attachment is obsolete: true
(Assignee)

Comment 5

8 years ago
Created attachment 486290 [details] [diff] [review]
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.
Attachment #486290 - Flags: review?(roc)
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?
(Assignee)

Comment 7

8 years ago
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.
Are we going to upstream stuff? Surely we need separate patches for that?
(Assignee)

Comment 9

8 years ago
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.
(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

8 years ago
What's the status of this? It's blocking 604307 which is important to quite a few people running Linux.
(Assignee)

Comment 13

8 years ago
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.
(Assignee)

Comment 14

8 years ago
Created attachment 489714 [details] [diff] [review]
Update to latest Chromium YCbCr code

Fixes the issue where the .patch file wasn't applied. Carried r+ forward.
Attachment #486290 - Attachment is obsolete: true
Attachment #489714 - Flags: review+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Whiteboard: [needs landing]
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
(Assignee)

Comment 15

8 years ago
http://hg.mozilla.org/mozilla-central/rev/64901a1fcf93
Whiteboard: [needs landing]
(Assignee)

Updated

8 years ago
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
> #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.
Depends on: 616778

Updated

3 years ago
See Also: → bug 1254872
You need to log in before you can comment on or make changes to this bug.