Closed Bug 1220082 Opened 9 years ago Closed 8 years ago

Keyframe animation prevents Firefox from rendering Background image change on hover correctly

Categories

(Core :: Layout: Images, Video, and HTML Frames, defect)

45 Branch
Unspecified
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: christoph-kruppe, Assigned: mattwoodrow)

References

Details

Attachments

(2 files, 5 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.80 Safari/537.36

Steps to reproduce:

1.) Make an simple HTML file with one element.
2.) Make an keyframe animation with some properties changing. (Does not matter which)
3.) give this element also an background image and an hover state that changes this background image to another or the background position for example.
4.) open HTML in Firefox and hover while keyframe animation is playing.


Actual results:

Hover is either not changing the background image or is triggering the change but does not get loose of the state after hovered. Its random if its the first or the last.


Expected results:

Background changing on hover and changing back if not hovering anymore.
Attached file test.html (obsolete) —
What kind of file format is the background image? Does it contain animations?

Do you also see the issue with the attached test.html?
Flags: needinfo?(christoph-kruppe)
The background images are both PNG files. I will provide an html file there i get this problem on monday. With yours i had no problem.
Flags: needinfo?(christoph-kruppe)
Attached file test_notworking.html (obsolete) —
Here now my testfile. It seems not to work if you transform or animate the Element with other things then background color. Its the setup how we use it in our complete produckt without the rest. But its not working in both.
Hi, 
I can reproduce this bug in FF Nightly 45.0a1 (2015-11-02) on Windows 7 x64.
Steps to reproduce:

1 Open the second attachment from Attachments, or go on this link https://bug1220082.bmoattachments.org/attachment.cgi?id=8681855 

2 Hover the mouse over the text

Actual results: 
Hover is either not changing the background image or is triggering the change but does not get loose of the state after hovered.

Expected results:
Background is changed when you hover the mouse over the text and changing back when you're not hovering.
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout: Images
Ever confirmed: true
OS: Unspecified → Windows 7
Product: Firefox → Core
Version: 41 Branch → 45 Branch
Attached file slightly simpler testcase (obsolete) —
Attachment #8681278 - Attachment is obsolete: true
Attachment #8681855 - Attachment is obsolete: true
I've done a bit of debugging of the simplified testcase, and some modified forms of it.

One thing I ruled out:  it's not related to the opacity starting off as 0, and a layer having only a nonzero opacity in the animation data and not its primary opacity field.  If I changed the 0 to 0.1, the bug is still present.

However, the bug is related to the contents of the transform being optimized into an ImageLayer (see "CanOptimizeToImageLayer", etc.).  If I stick an "x" in the div, the bug goes away.

I still need to work through how InvalidateImagesCallback (in layout/style/ImageLoader.cpp) is supposed to work when its aItem has an mOptLayer (and whether that aItem->Invalidate() is still effective).
So I'm curious how you think this ought to work.

What's happening here is that a background-image is being changed dynamically.  That background image has been optimized to an ImageLayer.  This means that the code in ImageLoader that invalidates things:
https://hg.mozilla.org/mozilla-central/file/7883e81f3c30/layout/style/ImageLoader.cpp#l320
isn't currently sufficient.  But the hack in attachment 8693149 [details] [diff] [review] does make it work, since then when we recycle the old image layer we invalidate it.

Should the aItem->Invalidate() call in ImageLoader be sufficient to make this work, or should we need this hack or something similar?
Flags: needinfo?(matt.woodrow)
LayerProperties is supposed to capture changes to the layer tree and issue invalidations for it.

We call LayerProperties::CloneFrom [1] before modifying the layer tree to grab a snapshot of the initial state, and then LayerProperties::ComputeDifferences [2] after layer building to compute the changes.

ImageLayerProperties::ComputeChangeInternal is checking for a change of ImageContainer [3], so I'd expect that to be sufficient (and equivalent to the change your patch adds).


[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#1617
[2] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#1772
[3] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/LayerTreeInvalidation.cpp#466
Flags: needinfo?(matt.woodrow)
(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> ImageLayerProperties::ComputeChangeInternal is checking for a change of
> ImageContainer [3], so I'd expect that to be sufficient (and equivalent to
> the change your patch adds).

That does in fact happen, and it returns an appropriate looking invalid rect up to nsDisplayList::PaintRoot, which then throws it away because shouldInvalidate is false.  Presumably something along the way should have invalidated the layers (though I don't see what)?  Or should shouldInvalidate have been true?
Flags: needinfo?(matt.woodrow)
(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #11)
 
> That does in fact happen, and it returns an appropriate looking invalid rect
> up to nsDisplayList::PaintRoot, which then throws it away because
> shouldInvalidate is false.  Presumably something along the way should have
> invalidated the layers (though I don't see what)?  Or should
> shouldInvalidate have been true?

shouldInvalidate is true when we need to dispatch the invalid region to the OS in order to get a paint event.

This is always false with OMTC, as we drive painting ourselves in that case.

For OTMC, we run the layer tree comparison again on the compositor side (necessary, since OMTA could have made further changes since the client side comparison) at [1].

Only CompositorD3D11 and BasicCompositor actually use these results to restrict compositing (is this bug compositor specific?), and it seems like the ImageLayerProperties::ComputeChangeInternal method is failing to detect the change this time.


[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/LayerManagerComposite.cpp#345
Flags: needinfo?(matt.woodrow)
(In reply to Matt Woodrow (:mattwoodrow) from comment #12)
> For OTMC, we run the layer tree comparison again on the compositor side
> (necessary, since OMTA could have made further changes since the client side
> comparison) at [1].

This appears not to work, because on the compositor side, the ImageContainer* is always null.  (That is, both ImageLayerProperties::mContainer and ImageLayerProperties::mLayer.mContainer are null for every call to mozilla::layers::ImageLayerProperties::ComputeChangeInternal.)
And I don't see anything designed to map container identity from child to parent.  ImageLayerAttributes in LayersMessages.ipdlh doesn't have anything to represent the ImageContainer's identity.
Flags: needinfo?(matt.woodrow)
ImageHost is the closest compositor side equivalent to ImageContainer, and we check the ProducerID() and FrameID() of this.

It looks like setting a new image on an existing ImageContainer [1] will increment the frame id, which is transmitted to the parent.

However, the producer id defaults to 0 unless explicitly specified [2], so switching ImageContainers might not be detected (if we only ever have one image per container).

I think we should use AllocateProducerID() to allocate a fallback producer id for each ImageContainer instance and use that instead of 0.

Alternatively we could try make sure that we re-use the ImageContainer and only have the Image change, but that might be considerably more complex (and involve refactoring imagelib).


[1] http://mxr.mozilla.org/mozilla-central/source/dom/media/VideoFrameContainer.cpp#40
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ImageContainer.h#329
Flags: needinfo?(matt.woodrow)
Assignee: nobody → matt.woodrow
Attachment #8693149 - Attachment is obsolete: true
Attachment #8706222 - Flags: review?(seth)
Attachment #8706222 - Flags: review?(roc)
The assertion is being removed because with animated images the frame id resets with each iteration of the animation.
Comment on attachment 8706222 [details] [diff] [review]
Assign frame ids and producer ids to animated Images

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

::: gfx/layers/ImageContainer.cpp
@@ -191,5 @@
>    if (!aImages.IsEmpty()) {
> -    NS_ASSERTION(mCurrentImages.IsEmpty() ||
> -                 mCurrentImages[0].mProducerID != aImages[0].mProducerID ||
> -                 mCurrentImages[0].mFrameID <= aImages[0].mFrameID,
> -                 "frame IDs shouldn't go backwards");

I don't think we should remove this. ImageHost has logic that depends on frameIDs advancing. So does the composition stats logic in ImageContainer. The latter might work OK for animated images, but the ImageHost logic for "Ignore frames before a frame that we already composited" seems scary.

Instead we need to generate fresh frame IDs for repeating animated images. This should be doable by storing a frame ID in RasterImage.
Attachment #8706222 - Flags: review?(roc)
Attachment #8706222 - Attachment is obsolete: true
Attachment #8706222 - Flags: review?(seth)
Comment on attachment 8706735 [details] [diff] [review]
Assign frame ids and producer ids to animated Images v2

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

This all seems reasonable to me.

If, in the long term, it'd be better to ensure that we keep reusing the same ImageContainer, it'd be good to file a followup. (As you mention, it's not trivial, since we hold a weak pointer to the ImageContainer precisely to allow image surfaces to be discarded when the layer system doesn't need them anymore.)

::: image/RasterImage.h
@@ +367,5 @@
>    // A weak pointer to our ImageContainer, which stays alive only as long as
>    // the layer system needs it.
>    WeakPtr<layers::ImageContainer> mImageContainer;
>  
> +  layers::ImageContainer::ProducerID mImageProducerID;

Please add a comment for these fields. ("Producer and frame IDs for mImageContainer"?)
Attachment #8706735 - Flags: review?(seth) → review+
Backed out for bustage:

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/fa7a5698fced

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a5d2e586777c
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=22948363&repo=mozilla-inbound
08:03:43     INFO -  Unified_cpp_dom_media_webrtc0.o
08:03:46     INFO -  In file included from /builds/slave/m-in-m64-000000000000000000000/build/src/image/imgRequest.cpp:17:
08:03:46     INFO -  /builds/slave/m-in-m64-000000000000000000000/build/src/image/RasterImage.h:372:47: error: expected ';' at end of declaration list
08:03:46     INFO -    layers::ImageContainer::FrameID mLastFrameID
08:03:46     INFO -                                                ^
08:03:46     INFO -                                                ;
08:03:46     INFO -  1 error generated.
08:03:46     INFO -  make[6]: *** [imgRequest.o] Error 1
08:03:46     INFO -  make[5]: *** [image/target] Error 2
Flags: needinfo?(matt.woodrow)
https://hg.mozilla.org/mozilla-central/rev/b6c044f63195
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Flags: needinfo?(matt.woodrow)
Product: Core → Core Graveyard
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.