590252 - Don't send incremental invalidations on image redecodes

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Bobby Holley (:bholley)]

Part of bug 516320.

Comment 1

[Bobby Holley (:bholley)]

Attachment: part 1 - v1 - Add framework to Decoder superclass to handle invalidations

We want to be smarter about invalidations, but it's much easier to do it through the superclass. First we add a framework for this.

Comment 2

[Bobby Holley (:bholley)]

Attachment: part 2 - v1 - Make decoder implementations use the Decoder superclass invalidation framework.

...then we make the decoders use it.

Comment 3

[Bobby Holley (:bholley)]

Attachment: part 3 - v1 - Only flush invalidations when we need to

Currently we send invalidations way too often, and they aren't cheap (according to roc), even though the repaints are coalesced. This patch makes us smarter in that area.

Comment 4

[Bobby Holley (:bholley)]

Attachment: part 4 - v1 - Don't do progressive display on redecodes

Beltzner thought that this would be better UX than progressive display.

Comment 5

[Bobby Holley (:bholley)]

Attachment: part 5 - v1 - Make RequestDecode() SyncDecode() on images that are small enough.

This fixes the flicker issue for the most part. For images with source data size less than 150k, we turn RequestDecode() into SyncDecode(). So big images still pop in asynchronously, but we don't have a flicker effect on most regular sized images.

One crappy thing about this patch is that it means that we sometimes end up decoding _more_ on tab switch than we did in FF3.6, because the document tracker calls RequestDecode() on _everything_ when we tab over, not just things in the visible viewport. In theory we should be able to be smarter about it, and I'm going to work on that next. But given the impending beta, I wanted to get a patch stack up that fixes the flicker issue right away.

Comment 6

[Joe Drew (not getting mail)]

Comment on attachment 469654 [details] [diff] [review]
part 2 - v1 - Make decoder implementations use the Decoder superclass invalidation framework.


>-  // Offset to the frame position
>-  // Only notify observer(s) for first frame
>-  if (!mGIFStruct.images_decoded && mObserver) {
>-    PRUint32 imgCurFrame = mImage->GetCurrentFrameIndex();
>-    mObserver->OnDataAvailable(nsnull, imgCurFrame == PRUint32(mGIFStruct.images_decoded), &r);
>-  }

This means we're going to do sub-frame invalidations for animated images, right? We don't want that, I think.


>-    // Tell the image that it's data has been updated 

r+ strictly on the basis of removing this "it's" from the codebase.

Comment 7

[Joe Drew (not getting mail)]

Comment on attachment 469655 [details] [diff] [review]
part 3 - v1 - Only flush invalidations when we need to


>+    mInDecoder = PR_TRUE;
>+    mDecoder->FlushInvalidations();
>+    mInDecoder = PR_FALSE;

This pattern is pretty common in this patch, and it's quite ugly. Is it necessary?

Also, it wasn't immediately obvious to me how the FlushInvalidations calls would result in us doing progressive decoding. It would be nice to have some comments to say how much of a frame is expected/known to be complete when at each FlushInvalidations call.

Comment 8

[Joe Drew (not getting mail)]

Comment on attachment 469663 [details] [diff] [review]
part 5 - v1 - Make RequestDecode() SyncDecode() on images that are small enough.


>+  // If it's a smallish image, it's not worth it to do things async
>+  if (!mDecoded && !mInDecoder && mHasSourceData && (mSourceData.Length() < 150000))
>+    return SyncDecode();
>+

This should be a pref.

Comment 9

[Bobby Holley (:bholley)]

(In reply to comment #6)
> This means we're going to do sub-frame invalidations for animated images,
> right? We don't want that, I think.

I don't think it matters. An image frame doesn't become current until it's fully decoded, so we'll always be passing aCurrentFrame=false for non-first-frames. The only important consumer that implements OnDataAvailable is nsImageFrame, which is a no-op for non-current frames:

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsImageFrame.cpp#542

Who knows what XUL and the like do, but I don't think it matters - we're not doing anything incorrect, we just could invalidate more than we need to. They're not the common case, so whatever.

Comment 10

[Bobby Holley (:bholley)]

Attachment: part 4 - v2 - Don't do progressive display on redecodes

Added a patch with more comments. carrying over r+.

> >+    mInDecoder = PR_TRUE;
> >+    mDecoder->FlushInvalidations();
> >+    mInDecoder = PR_FALSE;
> 
> This pattern is pretty common in this patch, and it's quite ugly. Is it
> necessary?

At the moment it is, because we need to take different paths when we're re-entrant. We could probably architect it away, but that's a task for later.

Comment 11

[Bobby Holley (:bholley)]

Attachment: part 5 - v2 - Make RequestDecode() SyncDecode() on images that are small enough (uses a pref)

Added a patch making it a pref. Note that this is based on bug 590260.

Flagging joe for review.

Comment 12

[Bobby Holley (:bholley)]

Attachment: v2 - Only flush invalidations when we need to

Oops, I accidentally uploaded-and-carried-over part 4, which didn't need it. Doing it for part 3, which is what I intended.

Comment 13

[Bobby Holley (:bholley)]

Pushed to mozilla-central:
part 1: http://hg.mozilla.org/mozilla-central/rev/24ae66f16d44
part 2: http://hg.mozilla.org/mozilla-central/rev/5875f7dbf9c6
part 3: http://hg.mozilla.org/mozilla-central/rev/9cf06ffbf06e
part 4: http://hg.mozilla.org/mozilla-central/rev/17d4c9bde222
part 5: http://hg.mozilla.org/mozilla-central/rev/3df62472ba02

Resolving fixed.