1470447 - JPEG decoder should post an invalidation for each row

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, enhancement, P3)

Description

[Andrew Osmond [:aosmond] (he/him)]

Unlike the PNG/BMP decoders, the JPEG decoder batches up its PostInvalidation calls after it has decoded every row that it was able to depending on how much input was available to process. This can make the decoding/displaying seem more janky than it really is because of the delay and then display of a large chunk. Combining this with bug 1469964 ensures we split the texture upload in smaller chunks at a time, allowing scrolling to appear slightly smoother.

Comment 1

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0003-Bug-XXX-JPEG-decoder.patch

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8987071 [details] [diff] [review]
0003-Bug-XXX-JPEG-decoder.patch

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

::: image/decoders/nsJPEGDecoder.cpp
@@ +672,5 @@
>          }
>          if (mDownscaler) {
>            mDownscaler->CommitRow();
>          }
> +        PostInvalidationIfNeeded(top);

Is it worth adding something like, after the declaration of `top`, above:

auto invalidator = MakeScopeExit([&]() { PostInvalidationIfNeeded(top); });

and removing the calls to PostInvalidationIfNeeded, so that if new paths through the loop are added, somebody doesn't have to remember to add new PostInvalidationIfNeeded calls?  The current patch might be faster, not sure.

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Nathan Froyd [:froydnj] (away until 16 July 2018) from comment #2)
> Comment on attachment 8987071 [details] [diff] [review]
> 0003-Bug-XXX-JPEG-decoder.patch
> 
> Review of attachment 8987071 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/decoders/nsJPEGDecoder.cpp
> @@ +672,5 @@
> >          }
> >          if (mDownscaler) {
> >            mDownscaler->CommitRow();
> >          }
> > +        PostInvalidationIfNeeded(top);
> 
> Is it worth adding something like, after the declaration of `top`, above:
> 
> auto invalidator = MakeScopeExit([&]() { PostInvalidationIfNeeded(top); });
> 
> and removing the calls to PostInvalidationIfNeeded, so that if new paths
> through the loop are added, somebody doesn't have to remember to add new
> PostInvalidationIfNeeded calls?  The current patch might be faster, not sure.

I've since changed it to absorb the CommitRow call, so I think it will be hard to forget :).

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0003-Bug-1470447-JPEG-decoder-should-post-an-invalidation.patch, v2

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/853cf14dc462
JPEG decoder should post an invalidation for each row. r=tnikkel

Comment 6

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/853cf14dc462