Closed Bug 812602 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: ImageLib, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19
Tracking Status
firefox19 --- fixed

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Details

Attachments

(2 files)

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.
Attachment #682583 - Flags: review?(joe)
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
Attachment #682583 - Flags: review?(joe) → review+
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.
(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.
https://hg.mozilla.org/mozilla-central/rev/3d6eb774184a
Assignee: nobody → jmuizelaar
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
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....
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 on attachment 683691 [details] [diff] [review]
don't use the DecodeStyle enum namespace

Jeff reviewed this over my shoulder.
Attachment #683691 - Flags: review+
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.
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
(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 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
Attachment #683691 - Flags: approval-mozilla-aurora?
Attachment #683691 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.