584652 - Add support for libjpeg-turbo colorspace conversions

Status: NEW

(Core :: Graphics: ImageLib, defect)

Description

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch with JCS_EXTENSIONS (from Adam Tkac, <[e-mail]>)

Firefox fails to build with libjpeg-turbo library (a replacement of libjpeg). libjpeg-turbo defines a JCS_EXTENSIONS and Adam was so kind to provide a patch for it.

Comment 1

[Martin Stránský [:stransky] (ni? me)]

Attachment: fixed one.

Comment 2

[Justin Lebar (not reading bugmail)]

I think we'll want to fold this code into the patch I'm writing for bug 573948.

Comment 3

[Justin Lebar (not reading bugmail)]

Are there any other places where we should be using libjpeg-turbo-provided methods?

Comment 4

[Adam Tkac]

(In reply to comment #2)
> I think we'll want to fold this code into the patch I'm writing for bug 573948.

Although bug #573948 looks like same issue, in my opinion both issues are different.

If I understand correctly bug #573948 is about libjpeg-turbo integration by default on all platforms but this bug is about gaining advantages of direct YUV->ARGB conversion on platforms which currently use libjpeg-turbo. So in my opinion you can process both issues separately. Note this change is fully compatible with other JPEG libraries.

Comment 5

[Adam Tkac]

(In reply to comment #3)
> Are there any other places where we should be using libjpeg-turbo-provided
> methods?

I quickly checked xulrunner sources and following places can take advantages from libjpeg-turbo methods:

1. modules/libpr0n/encoders/jpeg/nsJPEGEncoder.cpp:nsJPEGEncoder::InitFromData
2. ipc/chromium/src/base/gfx/jpeg_codec.cc:JPEGCodec::Encode

I'm not sure about ipc/chromium/src/base/gfx/jpeg_codec.cc:JPEGCodec::Decode method. Although it converts data only to RGB, other methods whose are called after this one can reorder ARGB elements; in this case libjpeg-turbo's colorspace extensions can be useful as well.

Comment 6

[Justin Lebar (not reading bugmail)]

(In reply to comment #4)
> (In reply to comment #2)
> If I understand correctly bug #573948 is about libjpeg-turbo integration by
> default on all platforms but this bug is about gaining advantages of direct
> YUV->ARGB conversion on platforms which currently use libjpeg-turbo.

This would only affect platforms which have libjpeg-turbo and build FF to use the system jpeg library, right?

Comment 7

[Adam Tkac]

(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #2)
> > If I understand correctly bug #573948 is about libjpeg-turbo integration by
> > default on all platforms but this bug is about gaining advantages of direct
> > YUV->ARGB conversion on platforms which currently use libjpeg-turbo.
> 
> This would only affect platforms which have libjpeg-turbo and build FF to use
> the system jpeg library, right?

Exactly.

Comment 8

[Justin Lebar (not reading bugmail)]

Would you be disappointed if we removed support for external jpeg libraries when we landed libjpeg-turbo?  I'm not sure what the benefit is of integrating with arbitrary libjpegs, especially since we don't test that configuration.

Comment 9

[Martin Stránský [:stransky] (ni? me)]

I believe all distros want to build against system jpeg libraries, right? And some systems may have old libjpeg libraries...

Comment 10

[Wolfgang Rosenauer [:wolfiR]]

openSUSE currently uses system jpeg libraries. I wouldn't be happy if support for that would be dropped but I have no strong arguments against it (besides of the usual 'Linux doesn't work that way for valid reasons' ones).

Comment 11

[Justin Lebar (not reading bugmail)]

I've filed bug 586320 for us to discuss whether we want to disable system libjpeg, require libjpeg-turbo, or do something else.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

I'm not going be able to get to this review anytime soon.

Comment 13

[Justin Lebar (not reading bugmail)]

This would probably become a higher priority if you could demonstrate a significant perf win with this (or a new) patch.  I tested it briefly and didn't observe a large difference in render speed.

Comment 14

[Justin Lebar (not reading bugmail)]

Perhaps using JCS_EXT_XRGB would have a measurable impact, since it would let us skip the RGB -> FRGB conversion altogether in OutputScanLines.

Comment 15

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

What are we doing with this bug since bug #573948 landed ?

Comment 16

[Makoto Kato [:m_kato]]

Attachment: Part 1 - BGRA support on qcms

Comment 17

[Makoto Kato [:m_kato]]

Attachment: Part 2 Add GetCMSBGRA on gfxPlatform

Comment 18

[Makoto Kato [:m_kato]]

Attachment: Part 3 - Use libjpeg-turbo on little endian system

Comment 19

[Takanori MATSUURA]

(In reply to Makoto Kato from comment #18)
> Part 3 - Use libjpeg-turbo on little endian system

This patch cannot be applied after checking in
https://hg.mozilla.org/mozilla-central/rev/edac7428b38c

Comment 20

[Justin Lebar (not reading bugmail)]

I did some benchmarking on my Nexus S.  libjpeg-turbo 1.2 should be able to get ~10MP/s, but we get ~5MP/s in Firefox.  I think one of the culprits is our slow YCC --> FRGB conversion.

libjpeg-turbo lets us skip this entirely, which would likely help, perhaps significantly.

But I don't understand why we need all these changes to QCMS, at least to get the improvement on mobile, where color management is disabled.  Can't we tell libjpeg-turbo to output to RGBX/BRGX and be done with it?

Comment 21

[Makoto Kato [:m_kato]]

(In reply to Justin Lebar [:jlebar] from comment #20)
> I did some benchmarking on my Nexus S.  libjpeg-turbo 1.2 should be able to
> get ~10MP/s, but we get ~5MP/s in Firefox.  I think one of the culprits is
> our slow YCC --> FRGB conversion.
> 
> libjpeg-turbo lets us skip this entirely, which would likely help, perhaps
> significantly.
> 
> But I don't understand why we need all these changes to QCMS, at least to
> get the improvement on mobile, where color management is disabled.  Can't we
> tell libjpeg-turbo to output to RGBX/BRGX and be done with it?

bug 584652 about color space conversion.

Comment 22

[Makoto Kato [:m_kato]]

(In reply to Justin Lebar [:jlebar] from comment #20)
> I did some benchmarking on my Nexus S.  libjpeg-turbo 1.2 should be able to
> get ~10MP/s, but we get ~5MP/s in Firefox.  I think one of the culprits is
> our slow YCC --> FRGB conversion.
> 
> libjpeg-turbo lets us skip this entirely, which would likely help, perhaps
> significantly.
> 
> But I don't understand why we need all these changes to QCMS, at least to
> get the improvement on mobile, where color management is disabled.  Can't we
> tell libjpeg-turbo to output to RGBX/BRGX and be done with it?

If QCMS supports BGRA, we can reduce byte swap since libjpeg-turbo supports BGRA.  Of course, this fix reduces it for non-QCMS platform such as mobile.

Should we separate qcms issue and non-qcms issue?

Comment 23

[Justin Lebar (not reading bugmail)]

> Should we separate qcms issue and non-qcms issue?

I think that might be helpful, since this bug is a lot of code and not  likely to get reviewed quickly, whereas the non-QCMS case is much simpler.

Comment 24

[Bruno George Moraes]

Maybe some problems can be averted by looking into this related bug on webkit. https://bugs.webkit.org/show_bug.cgi?id=59670

Comment 25

[noel gordon]

You may include that code only with suitable attribution :) or send over a t-shirt or something - but yes, swizzles avoid the need to reformat pixels on the output side of libjpeg-turbo.  The format costs are significant: ~50% of the overall decoding time according to our measurements.

I see an in-tree modification to qcms here.  Do you plan to upstream this work? We are doing the exact same thing on htttp://crbug.com/143 because we need the BGRA support in qcms (due to libjpeg-turbo) and plan to upstream it. Seems that all this work will collide.  Is there a way for us to better co-ordinate our efforts here?

Comment 26

[Bruno George Moraes]

I'm not from the webkit project, i only connected the dots... Try the bug assignee in webkit.

Comment 27

[Justin Lebar (not reading bugmail)]

I'm afraid to ask, but what does this have to do with tab switching?

Comment 28

[Joe Drew (not getting mail)]

My understanding is that Jeff's making anything that shows up in his tab switching profiles (e.g., faster image decode, i.e. this bug) block the tab switching bug.

Comment 29

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 567975 [details] [diff] [review]
Part 1 - BGRA support on qcms

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

I would rather the output of qcms be a build time operation instead of duplicating the code. This should also eliminate the need for the chrome patches to qcms.

Comment 30

[Makoto Kato [:m_kato]]

okay, I will import google's patches

Comment 31

[Jeff Muizelaar [:jrmuizel]]

Take a look at bug 791422