Closed Bug 1130328 Opened 5 years ago Closed 5 years ago

Always invalidate nsImageFrame's ImageLayer when handling FRAME_UPDATE

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla38
Tracking Status
firefox36 + verified
firefox37 + verified
firefox38 + verified

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(2 files, 4 obsolete files)

Right now ComputeGeometryChangeForItem does not invalidate any ImageLayer which may be associated with an invalidated DisplayItem. It only invalidates the PaintedLayer, which does not cause the ImageLayer to get repainted.

This is the cause of a number of image-related bugs right now.
Here's an initial patch.

One point in particular I wasn't clear on: is there any point in invalidating
the PainterLayer at all? In this version of the patch I invalidate both the
PaintedLayer and the ImageLayer, but I'm not sure that that's useful.
Attachment #8560378 - Flags: review?(tnikkel)
Blocks: 1130314
Comment on attachment 8560378 [details] [diff] [review]
Invalidate ImageLayers in ComputeGeometryChangeForItem

Probably use AddInvalidRect here.
Attachment #8560378 - Flags: review?(tnikkel) → review?(matt.woodrow)
(In reply to Timothy Nikkel (:tn) from comment #2)
> Comment on attachment 8560378 [details] [diff] [review]
> Invalidate ImageLayers in ComputeGeometryChangeForItem
> 
> Probably use AddInvalidRect here.

AddInvalidRect works on nsIntRects, though, not nsIntRegions, and I think we need a region here.
(It might not be a bad idea to add Layer::AddInvalidRegion, though.)
OK, here's a cleaned up version of this patch.

It still looks to me like I'm missing a translation that's necessary to put the
invalidation rect in the layer's coordinate system. |combined.MoveBy(-shift)|
wasn't quite right, so I've left that out of this patch.

Matt, do you know what the translation I need to apply is?
Attachment #8560858 - Flags: review?(matt.woodrow)
(In reply to Seth Fowler [:seth] from comment #5)
> It still looks to me like I'm missing a translation that's necessary to put
> the
> invalidation rect in the layer's coordinate system.

I should clarify that here I'm talking about putting the invalidation rect in the _ImageLayer's_ coordinate system.
Attachment #8560378 - Attachment is obsolete: true
Attachment #8560378 - Flags: review?(matt.woodrow)
Lawrence, since this will fix multiple bugs that are tracked for 36, we should probably be tracking this too, right?
Flags: needinfo?(lmandel)
We generally don't 'invalidate' layers except for PaintedLayer, because LayerTreeInvalidation is meant to detect all other types of changes.

ImageLayerProperties::ComputeChangeInternal is meant to pick up the changes to the image, why isn't it?
(In reply to Matt Woodrow (:mattwoodrow) from comment #8)
> We generally don't 'invalidate' layers except for PaintedLayer, because
> LayerTreeInvalidation is meant to detect all other types of changes.
> 
> ImageLayerProperties::ComputeChangeInternal is meant to pick up the changes
> to the image, why isn't it?

Hmm, I didn't know that existed.

I won't be able to investigate in more detail until tomorrow, but it looks to me like ImageLayerProperties::ComputeChangeInternal has no way to detect changes to the content of an ImageContainer (which is what happens when we're downloading a large image and progressively decoding it). Here's the code:

https://dxr.mozilla.org/mozilla-central/source/gfx/layers/LayerTreeInvalidation.cpp?from=ImageLayerProperties&case=true#380

It doesn't seem like there's any check that would work. Here's the code that RasterImage uses to update the ImageContainer in these situations:

https://dxr.mozilla.org/mozilla-central/source/image/src/RasterImage.cpp#857

Looks like we'd need to be checking for a change to the ImageContainer's current image.

Offhand, the most obvious approach I see (assuming that there is a 1-to-1 mapping between ImageContainers and ImageLayers) is to add a new method ImageContainer::CurrentImageWasPainted() which will tell us if we've painted the current image yet. This can be implemented by checking ImageContainer::mPaintTime.
(In reply to Seth Fowler [:seth] from comment #9)
> 
> It doesn't seem like there's any check that would work. Here's the code that
> RasterImage uses to update the ImageContainer in these situations:
> 
> https://dxr.mozilla.org/mozilla-central/source/image/src/RasterImage.cpp#857
> 
> Looks like we'd need to be checking for a change to the ImageContainer's
> current image.

Right, I think we should add that. Image's have a unique serial number that we could use to compare instead of holding an extra pointer.

That would be my preferred approach, but don't spent too long on it if it turns out to be problematic. The current patch isn't too bad (it's similar to how we call InvalidateLayer for 'explicit' ImageLayers).

> 
> Offhand, the most obvious approach I see (assuming that there is a 1-to-1
> mapping between ImageContainers and ImageLayers) is to add a new method
> ImageContainer::CurrentImageWasPainted() which will tell us if we've painted
> the current image yet. This can be implemented by checking
> ImageContainer::mPaintTime.

I don't think we would have that 1:1 mapping if someone used the same image multiple times (and more than one ended up getting layerized).
(In reply to Seth Fowler [:seth] from comment #7)
> Lawrence, since this will fix multiple bugs that are tracked for 36, we
> should probably be tracking this too, right?

Makes sense to me. In the future, please set the tracking? flag for the release as this will add the bug to our regular triage query. (I already set the flag on this bug.) Note that we are going to build with Beta 8 today so it is very late to take fixes for 36 at this point. We need a reviewed and tested patch immediately if we're going to proceed with the fix in 36.

Tracking as this blocks a number of bugs that are already tracking.
Flags: needinfo?(lmandel)
(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> Right, I think we should add that. Image's have a unique serial number that
> we could use to compare instead of holding an extra pointer.

Ah, that's handy! I'll use that.

> I don't think we would have that 1:1 mapping if someone used the same image
> multiple times (and more than one ended up getting layerized).

Absolutely, that's a good point.
(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> > Looks like we'd need to be checking for a change to the ImageContainer's
> > current image.
> 
> Right, I think we should add that. Image's have a unique serial number that
> we could use to compare instead of holding an extra pointer.
> 
> That would be my preferred approach, but don't spent too long on it if it
> turns out to be problematic. The current patch isn't too bad (it's similar
> to how we call InvalidateLayer for 'explicit' ImageLayers).

I implemented this, but it doesn't work. I haven't determined exactly why, but I added logging to print out the ImageContainer being examined for each call to ImageLayerProperties::ComputeChangeInternal, and I never see the container we're creating in RasterImage. Indeed, almost all calls to that method involve a null ImageContainer, which seems odd to me.

So at this point I'm back to the original patch in this bug, but there's still an issue that I don't understand how to fix. I'm using this testcase to test the patch:

https://bug795722.bugzilla.mozilla.org/attachment.cgi?id=666410

Sometimes after shift-reloading and clicking on the link, the entire image will be displayed except for a rectangle of pixels down the left side of the image, which remain white. Sometimes this doesn't happen.

I thought this might be an issue with an extra translation in |combined| shifting the invalidation region away from the left side of the layer. I tried subtracting the offset of the display item's bounds, which resulted in |combined|'s bounds having an offset of (0,0) (which seems like what we'd want in this case), but it didn't fix the problem.

Matt, do you have any idea what's going wrong here? Or why the ImageLayerProperties::ComputeChangeInternal approach might not be working?

Unfortunately at this point the only approach I've found that works 100% has been to modify nsImageFrame and replace the calls to InvalidateFrame and InvalidateFrameWithRect with calls to InvalidateLayer at this point:

https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsImageFrame.cpp#665

I suspect that change would not get an r+, though, since it seems like it's going to significantly increase the amount of repainting we'll do while an image is loading.
Flags: needinfo?(matt.woodrow)
OK, after discussion with Matt, we reached the conclusion that because we don't
have the ImageContainer objects available on the compositor side, implementing
the ImageLayerProperties::ComputeChangeInternal approach is not easy in the
short term. In the long term it'd be nice to make that work, though.

For now, it looks like we need to take the ComputeGeometryChangeForItem
approach. Last night I was still having issues with the translation being wrong.
Matt suggested I try using |layerData->mTranslation|, but that turned out to be
zero for the layers in question here. What I determined was that
|geometry.mBounds.x| and |geometry.mBounds.y| *do* indeed work. I thought I had
tried this last night, but I suspect that I made some sort of mistake and was
not actually applying the translation correctly - it was late at night, after
all.

So we have a fix for this issue!
Attachment #8562412 - Flags: review?(matt.woodrow)
Attachment #8560858 - Attachment is obsolete: true
Attachment #8560858 - Flags: review?(matt.woodrow)
Comment on attachment 8562412 [details] [diff] [review]
Invalidate ImageLayers in ComputeGeometryChangeForItem

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

::: gfx/layers/Layers.h
@@ +1545,5 @@
>    /**
>     * Adds to the current invalid rect.
>     */
>    void AddInvalidRect(const nsIntRect& aRect) { mInvalidRegion.Or(mInvalidRegion, aRect); }
> +  void AddInvalidRect(const nsIntRegion& aRegion) { mInvalidRegion.Or(mInvalidRegion, aRegion); }

AddInvalidRegion
Attachment #8562412 - Flags: review?(matt.woodrow) → review+
Thanks for the quick review! This version of the patch renames the new
AddInvalidRect overload to AddInvalidRegion, as per the review comments.
Attachment #8562412 - Attachment is obsolete: true
Try job here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=86b99acee0a1
Flags: needinfo?(matt.woodrow)
Comment on attachment 8562434 [details] [diff] [review]
Invalidate ImageLayers in ComputeGeometryChangeForItem

Unfortunately, this patch fixed one testcase but not all of the testcases we're trying to fix in this bug. I had no luck extending it to fix those testcases.

Especially given how short on time we are, the best bet seems instead to switch to calling InvalidateLayer() whenever we get a FRAME_UPDATE notification in nsImageFrame.

The main reason we don't always call InvalidateLayer() is that it can cause us to do more painting than necessary, since if there's no layer it will simply invalidate the entire frame. If we want to always call InvalidateLayer(), then, the way forward is to pass enough information to InvalidateLayer() to let us only invalidate the changed area of the frame.

Matt and I discussed passing an optional transform to InvalidateLayer(), but I realized once I dug into this that some of the calculations nsImageFrame does in SourceRectToDest() are nonlinear. Instead, I decided to make InvalidateLayer() accept an optional frame-space damage rect.

Patches forthcoming.
Attachment #8562434 - Attachment is obsolete: true
Here's the patch that updates nsIFrame::InvalidateLayer's interface.
Attachment #8562522 - Flags: review?(matt.woodrow)
In this patch nsImageFrame gets updated to always use InvalidateLayer when
handling FRAME_UPDATE.

This rendered some existing code dead. FrameChanged() becomes essentially the
same as the normal case, so I moved the two differences it had - the visibility
check and the lack of a need to invalidate the TYPE_ALT_FEEDBACK display item -
into OnFrameUpdate() and the new helper method InvalidateSelf(). Without
FrameChanged(), we also didn't need the imgIContainer overload of
IsPendingLoad().
Attachment #8562526 - Flags: review?(tnikkel)
Summary: ComputeGeometryChangeForItem should invalidate any ImageLayer associated with an invalidated DisplayItem → Always invalidate nsImageFrame's ImageLayer when handling FRAME_UPDATE
Attachment #8562522 - Flags: review?(matt.woodrow) → review+
Thanks for the review!

Try looks good too.
Attachment #8562526 - Flags: review?(tnikkel) → review+
Comment on attachment 8562522 [details] [diff] [review]
(Part 1) - Add support for an optional frame-space damage rect to nsIFrame::InvalidateLayer

Approval Request Comment
[Feature/regressing bug #]: Unknown. (Bug 1060869 aggravated this problem, but it didn't cause it.)
[User impact if declined]: Layerized images can end up only partially painted to the screen. It can appear to the user that the image never finishes loading.
[Describe test coverage new/current, TreeHerder]: Green on try. Pushed to m-i; will hopefully have landed on m-c by the time this is approved.
[Risks and why]: This is the lowest risk patch I could produce that resolves the bug. nsImageFrame already used InvalidateLayer for invalidation for animated images; these patches make it possible for nsImageFrame to use InvalidateLayer for invalidation during the initial decode as well. We are shifting from one invalidation path to another existing one which is already used in many other places.
[String/UUID change made/needed]: None.

Note that these patches fix multiple bugs tracked for 36. See the bugs which this one blocks. This is a pretty serious bug, so I really hope we can take this patch even though it's pretty late in the cycle.
Attachment #8562522 - Flags: approval-mozilla-beta?
Comment on attachment 8562526 [details] [diff] [review]
(Part 2) - Update nsImageFrame to always use InvalidateLayer when handling FRAME_UPDATE

We need part 2 as well.

Note that these patches apply just fine on beta, by the way. No rebasing should be necessary.
Attachment #8562526 - Flags: approval-mozilla-beta?
Seth, I guess we want the patch also in aurora, right? :)
Flags: needinfo?(seth)
(In reply to Sylvestre Ledru [:sylvestre] from comment #26)
> Seth, I guess we want the patch also in aurora, right? :)

I'm not requesting uplift to Aurora for a reason. This particular patch is probably OK, but in general, a lot of the bugs I've been working on lately have dependencies, and I need to go through and triage them to determine which *other* patches are required for uplift.

It will be up to you and the other release drivers, but I'm pretty much aiming to uplift *everything* I've done in 38 to 37. It's almost all correctness and stability work.

I'm hoping to make the first batch of uplift requests for Aurora later today.

But this is a conversation we can have outside of this bug.
Flags: needinfo?(seth)
Comment on attachment 8562522 [details] [diff] [review]
(Part 1) - Add support for an optional frame-space damage rect to nsIFrame::InvalidateLayer

OK, I've had a chance to triage these patches, and they should work fine on Aurora. Let's go ahead and uplift them there too.
Attachment #8562522 - Flags: approval-mozilla-aurora?
Attachment #8562526 - Flags: approval-mozilla-aurora?
Attachment #8562522 - Flags: approval-mozilla-beta?
Attachment #8562522 - Flags: approval-mozilla-beta+
Attachment #8562522 - Flags: approval-mozilla-aurora?
Attachment #8562522 - Flags: approval-mozilla-aurora+
Comment on attachment 8562526 [details] [diff] [review]
(Part 2) - Update nsImageFrame to always use InvalidateLayer when handling FRAME_UPDATE

I must admit that I am not super happy to have a big patch like that that late in the cycle.
However, the regressions are quite important.
Attachment #8562526 - Flags: approval-mozilla-beta?
Attachment #8562526 - Flags: approval-mozilla-beta+
Attachment #8562526 - Flags: approval-mozilla-aurora?
Attachment #8562526 - Flags: approval-mozilla-aurora+
Can someone from QE verify that it is fixed in 38 (please note the various blocked bugs which have some various STR) ?
Keywords: qaurgent, verifyme
Depends on: 1132427
(In reply to Sylvestre Ledru [:sylvestre] from comment #31)
> Can someone from QE verify that it is fixed in 38 (please note the various
> blocked bugs which have some various STR) ?

I've verified all bugs blocked by this, and indeed they appear fixed using the latest Firefox 38 Nightly (BuildID=20150211222327) on Windows 7 x64 (e10s on or off). There were also no issues resizing the browser window. I reproduced the issues in these bugs in 35.0.1 and/or 38 from 2015-01-12.

I would like to do some additional testing on Mac and Ubuntu as well, and also cover some other pages with images. Sylvestre, could you please let me know whether this fix will stick and I should continue with additional testing? Or will this be backed out because of bug 1132427?
Flags: needinfo?(sledru)
Florin, sorry, I don't know yet... :(
Flags: needinfo?(sledru)
Flags: qe-verify+
I've spent some additional time today verifying this also on:
- latest Firefox 37 Aurora - BuildID=20150213004027
- Firefox 36 Beta 9 - BuildID=20150212154903

Verification covered Windows 7 x64, Mac OS X 10.9.5, and Ubuntu 12.04 x86. Latest Nightly 38 was also verified on Mac and Ubuntu.

I covered all 5 blocked bugs, plus some other websites with images, but did not find any other issues.

I'm marking this as verified, and unless this will be backed out, I guess we can also close the 5 blocked bugs. Just let me know if I can go ahead and close them.
Sounds great. We will watch the first results with beta 9 if this introduced regressions or not and make a call for beta 10.
(In reply to Florin Mezei, QA (:FlorinMezei) from comment #36)
> I'm marking this as verified, and unless this will be backed out, I guess we
> can also close the 5 blocked bugs. Just let me know if I can go ahead and
> close them.

Thanks for verifying this fix, Florin!

Given that this seems to have stuck now, I think it's OK to close the blocked bugs.
You need to log in before you can comment on or make changes to this bug.