Closed Bug 1470447 Opened Last year Closed Last year

JPEG decoder should post an invalidation for each row

Categories

(Core :: ImageLib, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: aosmond, Assigned: aosmond)

Details

(Keywords: perf, Whiteboard: [gfx-noted] [qf:p3:f64])

Attachments

(1 file, 1 obsolete file)

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.
Attached patch 0003-Bug-XXX-JPEG-decoder.patch (obsolete) — Splinter Review
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.
Keywords: perf
Priority: -- → P3
Whiteboard: [gfx-noted] [qf]
Whiteboard: [gfx-noted] [qf] → [gfx-noted] [qf:p3:f64]
(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 :).
Assignee: nobody → aosmond
Attachment #8987071 - Attachment is obsolete: true
Attachment #8991012 - Flags: review?(tnikkel)
Attachment #8991012 - Flags: review?(tnikkel) → review+
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/853cf14dc462
JPEG decoder should post an invalidation for each row. r=tnikkel
https://hg.mozilla.org/mozilla-central/rev/853cf14dc462
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.