Closed Bug 1486278 Opened Last year Closed Last year

Video controls quickly reappears while fading

Categories

(Toolkit :: XUL Widgets, defect, P1)

Unspecified
Android
defect

Tracking

()

VERIFIED FIXED
mozilla64
Tracking Status
firefox-esr60 --- unaffected
firefox62 --- wontfix
firefox63 --- verified
firefox64 --- verified

People

(Reporter: filman230, Assigned: timdream)

References

Details

(Keywords: regression)

Attachments

(6 files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:63.0) Gecko/20100101 Firefox/63.0
Build ID: 20180826100553

Steps to reproduce:

1- Play a video
2- Tap anywhere on the screen to hide controls or wait for them to disappear


Actual results:

The controls begin to fade, then reappear a few milliseconds later and finally disappear completely.

Video of the bug : https://youtu.be/GrFNl1zXwn4


Expected results:

The controls shouldn't reappear.

It seems to also affect Firefox for Desktop.

mozregression --app=fennec --good=2018-04-3 --bad=2018-04-4

or

mozregression --app=fennec --repo=mozilla-inbound --good=a3ac84a960c7a64150c5302ebb6d72abc39bb46c --bad=c9db3f332a190addcc47ab54b0ccd5366948fd35
I was able to reproduce in Firefox 62.0 RC & Nightly 63.0a1 (2018-08-29) on Google Pixel C (Android 8.0.0)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Levente, would you be able to find a regression range for this?
Flags: needinfo?(levente.sacal)
Hi all,

The regression range that I have found is below:

-Last good build: build_date: 2018-04-04 10:19:31.065000

-First bad build: build_date: 2018-04-04 18:39:22.881000
 Pushlog: https:https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ff0efa4132f0efd78af0910762aec7dcc1a8de66&tochange=c23c7481957f7b982cffc0ce1d25979c69ca2c2f
Flags: needinfo?(levente.sacal)
Sweet thanks Kevin and Levente.

Bug 1449532 looks like the obvious culprit here.
Blocks: 1449532
Flags: needinfo?(timdream)
Assignee: nobody → timdream
Status: NEW → ASSIGNED
Flags: needinfo?(timdream)
Priority: -- → P1
Do we know if this affects Fx63 as well with Web Animation enabled and the polyfill removed in bug 1485133?

If not, I don't think it makes sense to fix it for 61/62.
See Also: → 1485133
I can still see it on Fennec 64, this is a valid bug.
I've spent a few hours on this yesterday. We don't appear call into startFade() twice. I will try to reduce a test case to see what's the cause; I am not convinced that I've found a platform bug.
Attached file animation-test.html
Sadly, this is a defect of the Web Animation API specific on Fennec.

The test page flicks on Fennec but not on Desktop. Previously, both platforms flickers and the problem on Desktop was fixed in bug 1354501.

Bug 1354501 can also be suppressed by a workaround removed in bug 1485133. 

https://hg.mozilla.org/mozilla-central/rev/c0c4120ab7c7#l3.18

but in my test, it seems to have no effect on Fennec.
Assignee: timdream → nobody
Blocks: 1354501
Status: ASSIGNED → NEW
Component: Audio/Video → DOM: Animation
OS: Unspecified → Android
Product: Firefox for Android → Core
Version: Firefox 61 → Trunk
I thought there was already a bug for it but I can't find it now.

In part, 'finished' was never intended for chaining animations (but more for doing cleanup, triggering other steps etc.)--forthcoming GroupEffects were intended for chaining effects. That said, I'm sure a lot of people will try to use it for that so we should fix this.

For main thread animations, this should work without flicker since we should end up running the 'finished' callback in the first frame after the animation finishes. For compositor animations, however, that's not guaranteed to happen.

That's largely ameliorated by forcing the fill mode to 'forwards' for compositor animations until the main thread has a chance to drop them:

  https://searchfox.org/mozilla-central/rev/6d1ab84b4b39fbfb9505d4399857239bc15202ef/gfx/layers/AnimationHelper.cpp#560-569

Perhaps in this case we end up dropping the animations on the compositor before the main thread has a chance to catch up?

I don't think bug 1354501 should affect this (since it only relates to events, not promises) but it will certainly affect the polyfill implementation.

The other bug that was similar to this (but which I can't find now) claimed that Chrome doesn't flicker in some particular case but Firefox does so it might be possible to reproduce this on desktop too with different content.

It's nearly the weekend here but in the meantime this should be fixable by adding { fill: 'forwards' } to the timing parameters for the animation. However, to avoid forwards-filling animations from accumulating over time (bug 1253476--what I am working on right now) I'd also pair that with a call to `animation.cancel()` in the finished callback.
FWIW, I can't reproduce the bug using attachment 9012780 [details] on my Android device with Fennec 63.
Will try comment 11. Thank you so much for the suggestion!
Assignee: nobody → timdream
Status: NEW → ASSIGNED
Component: DOM: Animation → Audio/Video
Product: Core → Firefox for Android
No longer blocks: 1354501
This probably is a bug in DOM: Animation or Graphics ultimately. If we can find a way to reproduce this then we really should fix it.
I've filed bug 1494847 for that fact that the animation inspector is completely broken for this test case (in fact it actually breaks the animation itself).
I tried comment 11, it didn't work on my test phone. It's an old nexus 4 so I guess the speed of main thread v.s. the compositor makes happen to be reproducible...
Oh, I'm sorry, I thought this was about flicker at the end of the animation, not the start. Let me have a look next week.

What are the STR with the reduced test case? Is just pressing the button once supposed to trigger it?
Flags: needinfo?(bbirtles)
Sorry for the hasty triage on this one. I was just about the leave the office when it came in. The video looks completely different to the sort of issue I had in mind.

I start to wonder if this is a bug in our updatePlaybackRate implementation when we come to update the compositor. I don't suppose changing the order of play() and updatePlaybackRate() makes a difference?

(Note that if you're calling play() immediately after, you don't need to use updatePlaybackRate(), but can just set playbackRate directly. updatePlaybackRate() is an async method intended for smooth changes to the playback rate _during_playback_. If you haven't started playback yet or are paused, you can just set playbackRate directly.)

I really should just find a way to repro this so I don't need to keep bothering you.
Attached video IMG_0508.MOV
This is about the end of the animation alright.

STR:

1. Click on the test button

Expected:

1. A black box appears and fades to opacity: 0

Actual:

1. A black box appears and fades to opacity: 0, but flicks in the end.

The video shows the test case on a Nexus 4 with m-c (rev be037b5b6327) artifact debug build.
fill: 'backwards' works for the case?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #20)
> fill: 'backwards' works for the case?

Oh, that's very likely it! Thanks Hiro!

The code I linked to[1] adjusts the fill mode to avoid flicker when the main thread needs to catch up with a finished compositor animation but it assumes the animation is playing forwards whereas this animation is playing backwards.

If that's the problem, then this should be a really easy fix.

I brought an old phone with me to work today so I will see if I can reproduce and verify.

Also, for what it's worth, I found the other bug report I was looking for. Unfortunately it was an issues reported on stack overflow that has since been deleted so I can't check if it's the same problem or not.

[1] https://searchfox.org/mozilla-central/rev/6d1ab84b4b39fbfb9505d4399857239bc15202ef/gfx/layers/AnimationHelper.cpp#560-569
Flags: needinfo?(bbirtles)
For me at least, using fill: backwards fixes the flicker. I'm going to make up a patch to fix this in Gecko but if we're looking for something safer to uplift to beta, then we might want to use 'fill: backwards' in the content.
Component: Audio/Video → DOM: Animation
Keywords: regression
Product: Firefox for Android → Core
Yep, using 'fill:backwars' sounds a preferable fix for beta.
FWIIW, bug 1297312 seems to be relevant to this.
See Also: → 1297312
I looked at adding a test for this but it looks like we don't currently test for the "tweak the fill mode" behavior when filling forwards.

We have places where we work around it like:

  https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/layout/style/test/test_animations_omta.html#2382-2389

And:

  https://searchfox.org/mozilla-central/source/dom/animation/test/mozilla/test_style_after_finished_on_compositor.html

But the closest I can find to something that actually explicitly tests this is:

  https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/dom/animation/test/mozilla/file_transition_finish_on_compositor.html#53-60

Maybe I can try adding a couple of tests to test_style_after_finished_on_compositor.html. Hopefully they won't prove flaky. If they do, we can just drop them.
Given that for beta we are expecting doing a frontend workaround for this platform bug, it probably makes sense to split the platform work off into a separate bug.
Depends on: 1495350
Reverting this bug to be specifically about the front-end regression.
Component: DOM: Animation → XUL Widgets
Keywords: regression
Product: Core → Toolkit
Hi Tim, can you check if this patch helps? I don't have a Fennec build environment handy. If it does, this might be more suitable for uplifting to beta than bug 1495350.
Attachment #9013209 - Flags: feedback?(timdream)
Comment on attachment 9013209 [details] [diff] [review]
Possible workaround

Yeah, the patch fixes the problem. I am going to land this as-is.
Attachment #9013209 - Flags: review+
Attachment #9013209 - Flags: feedback?(timdream)
Attachment #9013209 - Flags: feedback+
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #29)
> Yeah, the patch fixes the problem. I am going to land this as-is.

Actually -- this need some adjustments and also port to the XBL binding before it could land in beta, so let me work on a completed patch instead.
I will rewrite the comment about bug 1253476 since I realized animation.cancel() is always needed for filling animations -- otherwise, the value of the animating property will forever be locked at the value at the finished fame, instead of applying the style from the stylesheet. I would like to make sure we don't accidentally remove that line when bug 1253476 has a fix (in spec/code).
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #31)

But again, we don't need to make the animation become a filling animation when bug 1495350 is fixed, so that line could be removed together.
For compositor animations, we don't guarantee the finished promise callback
to run in the first frame after the animation finishes. By setting
fill: both, we set the animation to fill until the main thread has a chance
to catch up.

A filling animation has to be cancelled explicitly, otherwise the
value of the animating style will be locked at the last frame of the animation.
Comment on attachment 9013376 [details]
Bug 1486278 - Workaround Bug 1495350 in video controls r=Gijs

Brian Birtles (:birtles) has approved the revision.
Attachment #9013376 - Flags: review+
Comment on attachment 9013376 [details]
Bug 1486278 - Workaround Bug 1495350 in video controls r=Gijs

:Gijs (he/him) has approved the revision.
Attachment #9013376 - Flags: review+
Attachment #9013376 - Attachment description: Bug 1486278 - Workaround Bug 1486278 in video controls r=Gijs → Bug 1486278 - Workaround Bug 1495350 in video controls r=Gijs
Pushed by tchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4392b5198fb7
Workaround Bug 1495350 in video controls r=Gijs,birtles
https://hg.mozilla.org/mozilla-central/rev/4392b5198fb7
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Needs uplift.
Flags: needinfo?(timdream)
Comment on attachment 9013376 [details]
Bug 1486278 - Workaround Bug 1495350 in video controls r=Gijs

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1449532

User impact if declined: Affects Fennec on slower phones any perhaps desktop on slower machines. The control bar of video controls will blink when fading out.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: See comment 0

List of other uplifts needed: Bug 1493089

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): local to video controls UI

String changes made/needed:
Flags: needinfo?(timdream)
Attachment #9013376 - Flags: approval-mozilla-beta?
Comment on attachment 9013376 [details]
Bug 1486278 - Workaround Bug 1495350 in video controls r=Gijs

P1 regression, uplift approved for 63 beta 13, thanks.
Attachment #9013376 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
QA Contact: enndeakin
QA Contact: enndeakin
I was able to reproduce this issue following the steps from the description on Release 62.0.3 and I can confirm that this is fixed on 63.0b13 and latest Nightly build 64.0a1 with Motorola Nexus 6(Android 7.1.1) and Sony Xperia Z5 Premium(Android 6.0.1).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.