Martin Stránský [:stransky] (ni? me) Reporter
	Description • 14 years ago

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.

	Justin Lebar (not reading bugmail)
	Comment 2 • 14 years ago

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

	Justin Lebar (not reading bugmail)
	Comment 3 • 14 years ago

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

	Adam Tkac
	Comment 4 • 14 years ago

(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.

	Adam Tkac
	Comment 5 • 14 years ago

(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.

	Justin Lebar (not reading bugmail)
	Comment 6 • 14 years ago

(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?

	Adam Tkac
	Comment 7 • 14 years ago

(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.

	Justin Lebar (not reading bugmail)
	Comment 8 • 14 years ago

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.

	Martin Stránský [:stransky] (ni? me) Reporter
	Comment 9 • 14 years ago

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

	Wolfgang Rosenauer [:wolfiR]
	Comment 10 • 14 years ago

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).

	Justin Lebar (not reading bugmail)
	Comment 11 • 14 years ago

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

	Jeff Muizelaar [:jrmuizel]
	Comment 12 • 14 years ago

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

	Justin Lebar (not reading bugmail)
	Comment 13 • 14 years ago

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.

	Justin Lebar (not reading bugmail)
	Comment 14 • 14 years ago

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

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

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

	Takanori MATSUURA
	Comment 19 • 13 years ago

(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

	Justin Lebar (not reading bugmail)
	Comment 20 • 13 years ago

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?

	Makoto Kato [:m_kato] Assignee
	Comment 21 • 13 years ago

(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.

	Makoto Kato [:m_kato] Assignee
	Comment 22 • 13 years ago

(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?

	Justin Lebar (not reading bugmail)
	Comment 23 • 13 years ago

> 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.

	Bruno George Moraes
	Comment 24 • 12 years ago

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

	noel gordon
	Comment 25 • 12 years ago

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?

	Bruno George Moraes
	Comment 26 • 12 years ago

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

	Justin Lebar (not reading bugmail)
	Comment 27 • 12 years ago

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

	Joe Drew (not getting mail)
	Comment 28 • 12 years ago

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.

	Jeff Muizelaar [:jrmuizel]
	Comment 29 • 12 years ago

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.

	Makoto Kato [:m_kato] Assignee
	Comment 30 • 12 years ago

okay, I will import google's patches

	Jeff Muizelaar [:jrmuizel]
	Comment 31 • 12 years ago

Take a look at bug 791422