Closed Bug 590252 Opened 9 years ago Closed 9 years ago

Don't send incremental invalidations on image redecodes

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta5+

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(5 files, 3 obsolete files)

Part of bug 516320.
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.
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.
Beltzner thought that this would be better UX than progressive display.
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.
Attachment #469653 - Flags: review?(joe)
Attachment #469654 - Flags: review?(joe)
Attachment #469655 - Flags: review?(joe)
Attachment #469656 - Flags: review?(joe)
Attachment #469663 - Flags: review?(joe)
Attachment #469653 - Flags: review?(joe) → review+
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.
Attachment #469654 - Flags: review?(joe) → review+
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.
Attachment #469655 - Flags: review?(joe) → review+
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.
Attachment #469663 - Flags: review?(joe) → review-
Attachment #469656 - Flags: review?(joe) → review+
(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.
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.
Attachment #469656 - Attachment is obsolete: true
Attachment #469920 - Flags: review+
Added a patch making it a pref. Note that this is based on bug 590260.

Flagging joe for review.
Attachment #469663 - Attachment is obsolete: true
Attachment #469921 - Flags: review?(joe)
Attachment #469921 - Flags: review?(joe) → review+
blocking2.0: --- → beta5+
Oops, I accidentally uploaded-and-carried-over part 4, which didn't need it. Doing it for part 3, which is what I intended.
Attachment #469655 - Attachment is obsolete: true
Attachment #469962 - Flags: review+
You need to log in before you can comment on or make changes to this bug.