812602 - Don't decode jpegs progressively when we have all the data

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: Don't decode progressively when we have all the data

Decoding progressively is slower because we duplicate work like color conversion and color correction for each outputted scan. We already suppress paints when have all of the data, so we shouldn't do the extra work.

Comment 1

[Joe Drew (not getting mail)]

Comment on attachment 682583 [details] [diff] [review]
Don't decode progressively when we have all the data

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

::: image/decoders/nsJPEGDecoder.cpp
@@ +83,5 @@
>  
>  
> +nsJPEGDecoder::nsJPEGDecoder(RasterImage &aImage, imgIDecoderObserver* aObserver, Decoder::DecodeStyle aDecodeStyle)
> + : Decoder(aImage, aObserver),
> +   mDecodeStyle(aDecodeStyle)

, mDecodeStyle(aDecodeStyle)

@@ +366,5 @@
>  
>      /*
>       * Don't allocate a giant and superfluous memory buffer
>       * when the image is a sequential JPEG.
>       */

update comment

::: image/decoders/nsJPEGDecoder.h
@@ +51,5 @@
>  
>  class nsJPEGDecoder : public Decoder
>  {
>  public:
> +  nsJPEGDecoder(RasterImage &aImage, imgIDecoderObserver* aObserver, Decoder::DecodeStyle aDecodeStyle);

may as well put & on the RasterImage as is the prevailing style

::: image/src/Decoder.h
@@ +123,5 @@
> +
> +  enum DecodeStyle {
> +      PROGRESSIVE,
> +      SEQUENTIAL
> +  };

Please document these flags

Comment 2

[Robert Lickenbrock [:rclick]]

It may be orthogonal to this bug, but I wonder if suppressing painting for progressive images isn't the best strategy in terms of perceived snappiness.

My thinking is that if we were to flush invalidations immediately after finishing the first pass, we may spend less time displaying nothing which might be perceived as being faster. Strictly speaking it would be slower though.

On the other hand, I don't have any data on how long it takes to finish a pass verses decoding the entire image sequentially. If decoding sequentially doesn't take significantly longer than a single pass, it probably isn't worth it.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

(In reply to Robert Lickenbrock [:rclick] from comment #2)
> It may be orthogonal to this bug, but I wonder if suppressing painting for
> progressive images isn't the best strategy in terms of perceived snappiness.
> 
> My thinking is that if we were to flush invalidations immediately after
> finishing the first pass, we may spend less time displaying nothing which
> might be perceived as being faster. Strictly speaking it would be slower
> though.
> 
> On the other hand, I don't have any data on how long it takes to finish a
> pass verses decoding the entire image sequentially. If decoding sequentially
> doesn't take significantly longer than a single pass, it probably isn't
> worth it.

I agree this is certainly worth considering.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d6eb774184a

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3d6eb774184a

Comment 6

[Gregory Szorc [:gps]]

This appears to have broken Clang SVN tip. This won't turn the build red until we upgrade the Clang revision on the builders. But, you should probably fix this ASAP.

I'll leave it up to the original bug owners on whether to back out and re-land or file a follow-up.

From my personal nightly Clang SVN tip builder (the http:// links don't work, sorry):

<http://jenkins.gregoryszorc.com:9000/job/mozilla-central-linux-x64-optimized-llvm-tip/ws/image/src/RasterImage.cpp>:2528:63: error: expected a class or namespace
                                   mHasBeenDecoded ? Decoder::DecodeStyle::SEQUENTIAL :
                                                     ~~~~~~~~~^
<http://jenkins.gregoryszorc.com:9000/job/mozilla-central-linux-x64-optimized-llvm-tip/ws/image/src/RasterImage.cpp>:2529:63: error: expected a class or namespace
                                                     Decoder::DecodeStyle::PROGRESSIVE);
                                                     ~~~~~~~~~^
2 errors generated.

In the directory  <http://jenkins.gregoryszorc.com:9000/job/mozilla-central-linux-x64-optimized-llvm-tip/ws/obj/image/src>
The following command failed to execute properly:
/home/jenkins-slave/workspace/llvm/bin/clang++ -o RasterImage.o -c -I../../dist/stl_wrappers -I../../dist/system_wrappers -include <http://jenkins.gregoryszorc.com:9000/job/mozilla-central-linux-x64-optimized-llvm-tip/ws/config/gcc_hidden.h> -DMOZ_GLUE_IN_PROGRAM -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES -DSTATIC_EXPORTABLE_JS_API -DNO_NSPR_10_SUPPORT -DEXCLUDE_SKIA_DEPENDENCIES -DOS_POSIX=1 -DOS_LINUX=1 -I<http://jenkins.gregoryszorc.com:9000/job/mozilla-central-linux-x64-optimized-llvm-tip/ws/image/decoders> -I<http://jenkins.gregoryszorc.com:9000/job/mozilla-central-linux-x64-optimized-llvm-tip/ws/content/svg/content/src> -I<http://jenkins.gregoryszorc.com:9000/job/mozilla-central-linux-x64-optimized-llvm-tip/ws/content/base/src> -I<http://jenkins.gregoryszorc.com:9000/job/mozilla-central-linux-x64-optimized-llvm-tip/ws/layout/svg> -I<http://jenkins.gregoryszorc.com:9000/job/mozilla-central-linux-x64-optimized-llvm-tip/ws/ipc/chromium/src> -I<http://jenkins.gregoryszorc.com:9000/job/mozilla-central-linux-x64-optimized-llvm-tip/ws/ipc/glue> -I../../ipc/ipdl/_ipdlheaders -I<http://jenkins.gregoryszorc.com:9000/job/mozilla-central-linux-x64-optimized-llvm-tip/ws/image/src> -I. -I../../dist/include -I<http://jenkins.gregoryszorc.com:9000/job/mozilla-central-linux-x64-optimized-llvm-tip/ws/obj/dist/include/nspr> -I<http://jenkins.gregoryszorc.com:9000/job/mozilla-central-linux-x64-optimized-llvm-tip/ws/obj/dist/include/nss> -fPIC -Qunused-arguments -Qunused-arguments -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wno-ctor-dtor-privacy -Wno-overlength-strings -Wno-invalid-offsetof -Wno-variadic-macros -Wno-c++0x-extensions -Wno-extended-offsetof -Wno-unknown-warning-option -Wno-return-type-c-linkage -Wno-mismatched-tags -Wno-long-long -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fshort-wchar -pthread -pipe -DNDEBUG -DTRIMMED -g -Os -freorder-blocks -fomit-frame-pointer -I<http://jenkins.gregoryszorc.com:9000/job/mozilla-central-linux-x64-optimized-llvm-tip/ws/obj/dist/include/cairo> -Qunused-arguments -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MF .deps/RasterImage.o.pp <http://jenkins.gregoryszorc.com:9000/job/mozilla-central-linux-x64-optimized-llvm-tip/ws/image/src/RasterImage.cpp>
make[6]: *** [RasterImage.o] Error 1
make[6]: *** Waiting for unfinished jobs....

Comment 7

[Koosha KM]

gcc cannot compile this change:

make[4]: Entering directory `/home/koosha/firefox-src/obj-i686-pc-linux-gnu/image/src'
/usr/bin/ccache c++ -o RasterImage.o -c -I../../dist/stl_wrappers -I../../dist/system_wrappers -include /home/koosha/firefox-src/config/gcc_hidden.h -DMOZ_GLUE_IN_PROGRAM -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DSTATIC_EXPORTABLE_JS_API -DNO_NSPR_10_SUPPORT -DEXCLUDE_SKIA_DEPENDENCIES  -DOS_POSIX=1 -DOS_LINUX=1  -I/home/koosha/firefox-src/image/decoders -I/home/koosha/firefox-src/content/svg/content/src -I/home/koosha/firefox-src/content/base/src  -I/home/koosha/firefox-src/layout/svg -I/home/koosha/firefox-src/ipc/chromium/src -I/home/koosha/firefox-src/ipc/glue -I../../ipc/ipdl/_ipdlheaders  -I/home/koosha/firefox-src/image/src -I. -I../../dist/include  -I/home/koosha/firefox-src/obj-i686-pc-linux-gnu/dist/include/nspr -I/home/koosha/firefox-src/obj-i686-pc-linux-gnu/dist/include/nss      -fPIC  -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wno-ctor-dtor-privacy -Wno-overlength-strings -Wno-invalid-offsetof -Wno-variadic-macros -Wcast-align -Wno-long-long -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fshort-wchar -pthread -pipe -DNDEBUG -DTRIMMED -g -Os -freorder-blocks  -fomit-frame-pointer  -I/home/koosha/firefox-src/obj-i686-pc-linux-gnu/dist/include/cairo    -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MF .deps/RasterImage.o.pp  /home/koosha/firefox-src/image/src/RasterImage.cpp
/home/koosha/firefox-src/image/src/RasterImage.cpp: In member function ‘nsresult mozilla::image::RasterImage::InitDecoder(bool)’:
/home/koosha/firefox-src/image/src/RasterImage.cpp:2528: error: ‘mozilla::image::Decoder::DecodeStyle’ is not a class or namespace
/home/koosha/firefox-src/image/src/RasterImage.cpp:2529: error: ‘mozilla::image::Decoder::DecodeStyle’ is not a class or namespace

Comment 8

[Joe Drew (not getting mail)]

Attachment: don't use the DecodeStyle enum namespace

Comment 9

[Joe Drew (not getting mail)]

Comment on attachment 683691 [details] [diff] [review]
don't use the DecodeStyle enum namespace

Jeff reviewed this over my shoulder.

Comment 10

[Joe Drew (not getting mail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b8b5ca28225

Comment 11

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Since there is no -std=gnu++0x in the command line this is probably another case of

http://llvm.org/pr13530

I will try to get time to work on it once I get to exit(0) enabled.

Comment 12

[Joe Drew (not getting mail)]

The patch I just committed should presumably fix everything we can fix here, i.e., 

error: ‘mozilla::image::Decoder::DecodeStyle’ is not a class or namespace

Comment 13

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #12)
> The patch I just committed should presumably fix everything we can fix here,
> i.e., 
> 
> error: ‘mozilla::image::Decoder::DecodeStyle’ is not a class or namespace

Correct, but this is (I think) the third time we hit http://llvm.org/pr13530 :-(

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1b8b5ca28225

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 683691 [details] [diff] [review]
don't use the DecodeStyle enum namespace

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 812602 (this bug)
User impact if declined: build failure with some compilers due to the first patch in this bug.
Testing completed (on m-c, etc.): the patch has landed on m-c a while ago
Risk to taking this patch (and alternatives if risky): none
String or UUID changes made by this patch: none

Comment 16

[Mike Hommey [:glandium]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c1e60736791c