1346432 - Regression Firefox hides all video frames but still plays audio without a way to disable it (sometimes)

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[684sigma]

I have a problem with Firefox Beta 53. It doesn't happen in Firefox ESR 45.
Sometimes downloading of a video stops because something breaks on the server, and it can't give me video chunks properly. In this case Firefox hides from me video frames, but still plays audio, without a way to stop the video.
It was very hard to find a working example of this bug, because it happens unpredictably, and as a user, I can't trigger issues on sever side.

1. Install and enable https://addons.mozilla.org/en-US/firefox/addon/inlinedisposition/
2. Open https://www.ssyoutube.com/watch?v=ZGqhhx8cxMg , Click "Download" button on the page
3. Disable internet connection and wait until the video displays network error

Result: Sound playback didn't stop, sound still goes from the video. Clicking pause button does nothing(!)
Expected: There should be no such situation. If video stops, there should be no sound coming from the video

Firefox ESR 45 just hides video and stops audio, and there're no issues.
Firefox Release 28 doesn't hide video and plays it, so regardless from what is considered to be "good" behavior, it was present on either Firefox ESR 45, or Firefox 28. Therefore it's regression.

Comment 1

[YF (Yang)]

WFM in Fx53.0b1 with the STR, the playback (video and sound) was paused with spinner (loading icon) shown, and resume the play after re-enabled the network.

Comment 2

[684sigma]

(In reply to YF (Yang) from comment #1)
> WFM in Fx53.0b1 with the STR, the playback (video and sound) was paused with
> spinner (loading icon) shown, and resume the play after re-enabled the
> network.

Re-enabling the network was not part of described scenario, please avoid that.

What part of the video was cached when you saw the spinner? What you're describing usually happens when a small part of video is cached, for example, if the internet connection is slow. Try the following

1. Install and enable https://addons.mozilla.org/en-US/firefox/addon/inlinedisposition/
2. Open https://www.ssyoutube.com/watch?v=ZGqhhx8cxMg , Click "Download" button on the page
3. Pause the video, wait until significant part of the video is cached, at least 10% of the video, but no more than 50%. Cached part of the video is displayed as a lightgray bar in timeline (timline is gray horizontal bar)
4. Play the video
5. Disable internet connection
6. Wait until the video displays network error

If it only displays network error only on my machine, I'll need some logging to prove that it really happens. Or request the video of the bug happening.

Comment 3

[YF (Yang)]

Thank you for detailed explanation, I now see this issue.

"Video playback aborted due to a network error. " from Firefox.

https://dxr.mozilla.org/mozilla-central/search?q=MEDIA_ERR_NETWORK

Comment 4

[Ray Lin[:ralin]]

Hey Alastor, 

Do we stop sound playback from playing when we encountered network disconnection, even in the condition that audio is completely downloaded/buffered? Thanks.

Comment 5

[Alastor Wu [:alwu]]

Media element would dispatch error to JS [1], but won't change its playing state. 
I think the audio might be stopped by js player?

[1] http://searchfox.org/mozilla-central/rev/c48398abd9f0f074c69f2223260939e30e8f99a8/dom/html/HTMLMediaElement.cpp#5300

Comment 6

[Ray Lin[:ralin]]

(In reply to Alastor Wu [:alwu] from comment #5)

No, media controls only display error message without manipulating media object. I suspect we could reproduce this issue without "controls" attached, but we do stop both video and audio playback(decoder) when error, right?


(In reply to 684sigma from comment #0)
> Result: Sound playback didn't stop, sound still goes from the video. Clicking pause button does nothing(!)
IIRC, we hide "controlBar" from users once error occurred. I don't understand why pause button should be clickable at that moment.

Comment 7

[684sigma]

Attachment: Beta 52 notifies user about neterror and playback continues, by reporter.png

I'm worried for this bug. Hopefully this comment will clarify the situation for everybody including myself.


(In reply to Ray Lin[:ralin] from comment #6)
> but we do stop both video and audio playback(decoder) when error, right?

I'll describe what happens in UI, and why this bug is regression:
ESR 45 - Video & audio disappear. Video controls stay visible. There's no way to do anything with already cached video chunk.
Beta 52 - Audio isn't stopped, video frames are visible below transparent overlay. User can stop and continue the already downloaded video chunk. All video controls are working as expected.
Beta 53 - Audio isn't stopped, video is not stopped: video frames are simply covered by opaque overlay (you can check it by hiding element [anonid="statusOverlay"] in video controls). Play/stop button doesn't work as expected, hotkeys do.
Nightly 55 - Same as Beta 53

From user perspective, the best option is UI of Beta 52: it notifies user that net error happened, and allows to manipulate the already downloaded part of the video. Let say, I have slow internet connection, and I'm watching a video which is loaded by 95%. Or I opened a tab with video from a slow server in a new tab, hoping that it'll be fully loaded when I visit that tab.
If browser terminates the video, in both cases I have to reload tab again, and probably fail again. That's a lot of time loss.


(In reply to Ray Lin[:ralin] from comment #6)
> (In reply to 684sigma from comment #0)
> > Result: Sound playback didn't stop, sound still goes from the video. Clicking pause button does nothing(!)
> IIRC, we hide "controlBar" from users once error occurred. I don't
> understand why pause button should be clickable at that moment.

I don't see any reason to hide controls, they're required to play/pause the downloaded video chunk and navigate back/forward.
Somebody improved this between ESR 45 and Beta 52 after all... Is that even necessary to notify the user about error?
Ray, if I understand correctly, you're developing front-end (video controls), so I'm waiting for your comment on this.

Last thing: if your final decision will be to terminate the video, please consider doing it in front-end, so I could continue playback by executing "video.play()" if I really need this.

Comment 8

[Ray Lin[:ralin]]

(In reply to 684sigma from comment #7)

Thank you, it's my fault misunderstanding the problem....

To solve it, I guess we need adjust the opacity of opaque overlay to increase the visibility of video frame, in the meanwhile, overlay should not suppress any operation on control bar. I agree it's totally make sense the Beta 52's approach is the best option for slow internet, and that is what I should have considered while discussing with visual designer prior to visual refresh in 53. Let me brief the problem to our visual designer to see how we can drive this fix to revert current behavior to similar to 52.

> Last thing: if your final decision will be to terminate the video, please consider doing it in front-end, so I could continue playback by executing "video.play()" if I really need this.
Except for covering video frame with a opaque overlay(error message), we won't do any extra to refrain user from using video.play() in front-end code. If we do, then there should have something back-end worry about.

If there are still something missing or misunderstood by me, please kindly correct me. :)

Comment 9

[Ray Lin[:ralin]]

> Result: Sound playback didn't stop, sound still goes from the video. 
It's caused by: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=060f80b690b8aaa5d927e03578673a3eff3b4c64&tochange=000dc91517d648344729bc8764aee1cca8e91e77

more precisely: https://hg.mozilla.org/mozilla-central/rev/d392ffb70683#l1.30

I initially expect video(including controls) should be frozen if any error occurred, as that is what video used to do before bug 1302656 landed. That is also the reason our visual designer designed a dark overlay for error message.

> Clicking pause button does nothing(!)
This should be a regression of bug 1302656, nonetheless, we need to file another bug for play button when video is still playable but has network error.


For bug 1271765, the regression is the dark overlay. Since TPE office won't be open until Wednesday, I need to wait then to ask Peko to decrease opacity of error message's background.

Comment 10

[Ray Lin[:ralin]]

(In reply to 684sigma from comment #7)

> Beta 52 - Audio isn't stopped, video frames are visible below transparent overlay. User can stop and continue the already downloaded video chunk. All video controls are working as expected.
I can confirm that for Beta 52, user could not stop downloaded video chunk by play button. The original play button is designed to stop working once error occurred[0], so I suspect why would be that case. 

I've filed another bug for that: bug 1352995

[0] https://dxr.mozilla.org/mozilla-central/rev/38894655c89e68bcd8f45d31a0d3005f2c2b53db/toolkit/content/widgets/videocontrols.xml#1282-1290

Comment 11

[Ray Lin[:ralin]]

Attachment: Bug 1346432 - Make the overlay of error page more transparent.

After bug 1302656 landed, video would continue even if error occurred.
Instead of convering a solid grey background over video, we should make it
more visible in order not to confuse users as audio would still plays out.

Review commit: https://reviewboard.mozilla.org/r/128794/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128794/

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8856869 [details]
Bug 1346432 - Make the overlay of error page more transparent.

https://reviewboard.mozilla.org/r/128794/#review131400

::: toolkit/themes/shared/media/videocontrols.css:48
(Diff revision 1)
>  .statusOverlay {
>    display: flex;
>    flex-direction: column;
>    justify-content: center;
>    align-items: center;
> -  background-color: rgb(80,80,80);
> +  background-color: rgb(80,80,80, .85);

Some errors, such as the loading stopped error, can still play the media that was downloaded.

Can you file a bug about adding the ability to hide the error overlay (maybe an X in the corner)?

Comment 13

[Ray Lin[:ralin]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12)
> Some errors, such as the loading stopped error, can still play the media
> that was downloaded.
> 
> Can you file a bug about adding the ability to hide the error overlay (maybe
> an X in the corner)?

Thank you Jared, I've filed bug 1355759 for that.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Autoland can't push this until all pending issues in MozReview are marked as resolved.

Comment 15

[Ray Lin[:ralin]]

Sorry, issue closed now :P

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/860fb6f3b303
Make the overlay of error page more transparent. r=jaws

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/860fb6f3b303

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Looks like we'll want to uplift this to 54 if possible. Too late for 53 at this point.

Comment 19

[Ray Lin[:ralin]]

Comment on attachment 8856869 [details]
Bug 1346432 - Make the overlay of error page more transparent.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1271765
[User impact if declined]: User can't not see ongoing video after error occurred, the video is hide from them by a solid grey overlay.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low risky
[Why is the change risky/not risky?]: one line CSS that change opacity of overlay background color.
[String changes made/needed]: none

Thanks!

Comment 20

[Gerry Chang [:gchang]]

Hi [e-mail],
Can you help verify if the issue is fixed as expected in the latest nightly?

Comment 21

[Ray Lin[:ralin]]

Sorry, I want to add a point in order not mislead [e-mail] if we want to verify this patch.

I've spun off another bug(bug 1352995) for play button problem, and I don't think this patch does completely solve 684sigma's problem. Please verify only whether the video is more visible than before in the nightly, thanks. 

Also, we'll provide additional button to close error message entirely in bug 1355759. Hope those would helps for your case.

Comment 22

[Gerry Chang [:gchang]]

Hi :ralin,
This looks like we need all 3 bugs to fix the issue. If that's the case, since this bug is not a new regression, can we let it ride the train and won't fix in 54?

Comment 23

[Ray Lin[:ralin]]

Yes. This is actually regressed by two bugs but I don't see it urgent to uplift this patch which only solve half of the problem. Thanks.

Comment 24

[Gerry Chang [:gchang]]

Comment on attachment 8856869 [details]
Bug 1346432 - Make the overlay of error page more transparent.

Per comment #22 & #23, let's let it ride the train and won't fix in 54. Beta54-.

Comment 25

[684sigma]

Attachment: Beta 52.png

Sorry for the delay, I was busy.

(In reply to Ray Lin[:ralin] from comment #21)
> I've spun off another bug(bug 1352995) for play button problem, and I don't
> think this patch does completely solve 684sigma's problem.

I recall now, I tested Spacebar key in Beta 52, and clicked on Play button in Nightly 55. So yes, play button wasn't a part of regression, it just never worked (at least in Beta 52 it didn't work)

> Please verify
> only whether the video is more visible than before in the nightly, thanks.

I confirm this, the video is more visible in Nightly (2017-05-09). (Beta 53 - not visible, Nightly (2017-05-09) - a bit visible)
But it's less visible than in Beta 52.

Comment 26

[684sigma]

Attachment: Nightly 55 (2017-05-09).png