739542 - Disable screen timeout when playing HTML5 <video> (webm, H.264)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Frank Wein [:mcsmurf]]

To reproduce:
1. Go to http://tagesschau.de/videoblog/zwischen_mittelmeer_und_jordan/videoblogschneider226.html
2. Play the HTML5 <video> (webm)

Actual results:
Default screen timeout turns the screen off after a while and the screen goes black

Expected results:
The screen timeout should be disabled while a video is playing

Tested with a mozilla-central nightly on a Samsung Galaxy S2, Android 2.3.4

Comment 1

[Aaron Train [:aaronmt]]

DId XUL have this?

I spotted http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoAppShell.java#1200 setKeepScreenOn() but it's been marked TODO when the Java compositor pieces were landing months ago.

Any update?

Comment 3

[Adrian Tamas (:AdrianT)]

Still an issue for html5 videos in fullscreen on both Aurora 15.0a2 2012-07-09 and Nightly 16.0a1 2012-06-09 on HTC Desire running Android 2.2.2. Updating flags to cover Firefox 15 and 16.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Brad - with HTML video becoming more viable, should we assign this to someone?

Comment 5

[Wesley Johnston (:wesj)]

We did do this in XUL Fennec and the interfaces still exist. See:

http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/common-ui.js#1161

Comment 7

[Chris Peterson [:cpeterson]]

We probably want to disable screen dimming when playing non-fullscreen video, too.

Related core bugs: bug 517870, bug 772347

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

This will disable the screen timeout for all dom fullscreen elements, including fullscreen video

Comment 9

[Chris Peterson [:cpeterson]]

Comment on attachment 674467 [details] [diff] [review]
patch

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

* Do we really want any fullscreen web page to wakelock the screen?
* Do we get a "DOMFullScreen:Stop" notification if the user opens Android's task switcher to switch apps?
* This patch duplicates some functionality already in GeckoApp.notifyWakeLockChanged().
* We probably want to wakelock the screen during non-fullscreen video playback, too.

::: mobile/android/base/GeckoApp.java
@@ +1000,5 @@
>                  focusChrome();
>              } else if (event.equals("DOMFullScreen:Start")) {
>                  mDOMFullScreen = true;
> +                PowerManager pm = (PowerManager) getSystemService(Context.POWER_SERVICE);
> +                mDOMFullScreenWakeLock = pm.newWakeLock(PowerManager.SCREEN_BRIGHT_WAKE_LOCK, "DOMFullScreen");

Do we need to worry about "DOMFullScreen:Start" being called more than once? If mDOMFullScreenWakeLock is not null, it should be released here. To avoid (theoretical) screen flashing, maybe it should be released after the second wakelock is acquired below:

@@ +1002,5 @@
>                  mDOMFullScreen = true;
> +                PowerManager pm = (PowerManager) getSystemService(Context.POWER_SERVICE);
> +                mDOMFullScreenWakeLock = pm.newWakeLock(PowerManager.SCREEN_BRIGHT_WAKE_LOCK, "DOMFullScreen");
> +                mDOMFullScreenWakeLock.acquire();
> +                Log.i("GeckoWakeLocck", "acquire");

s/GeckoWakeLocck/GeckoWakeLock/

@@ +1008,5 @@
>                  mDOMFullScreen = false;
> +                if (mDOMFullScreenWakeLock != null) {
> +                    mDOMFullScreenWakeLock.release();
> +                    mDOMFullScreenWakeLock = null;
> +                    Log.i("GeckoWakeLocck", "release");

s/GeckoWakeLocck/GeckoWakeLock/

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 674467 [details] [diff] [review]
patch

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

::: mobile/android/base/GeckoApp.java
@@ +1000,5 @@
>                  focusChrome();
>              } else if (event.equals("DOMFullScreen:Start")) {
>                  mDOMFullScreen = true;
> +                PowerManager pm = (PowerManager) getSystemService(Context.POWER_SERVICE);
> +                mDOMFullScreenWakeLock = pm.newWakeLock(PowerManager.SCREEN_BRIGHT_WAKE_LOCK, "DOMFullScreen");

I thought about "DOMFullScreen:Start" being sent twice after I submitted the patch and left for the night. I think this is best solved by just checking for null before creating a new one.

For screen flickering, I'm not terribly concerned since this only happens when you enter or exit fullscreen. But if we are concerned about that, the right way to handle it would be to pass ON_AFTER_RELEASE which resets the screen timeout when releasing the lock.

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

created bug 805017 to implement wake locking for fullscreen elements so this can focus on wake locking during video playback

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 13

[Chris Peterson [:cpeterson]]

Comment on attachment 677006 [details] [diff] [review]
patch

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

I changing all of nsHTMLMediaElement.cpp's references to the mPaused flag would be inconvenient, but hiding wakelock behavior behind overloaded operators seems too clever. Readers might be surprised that `mPaused = false` has significant side effects like acquiring a wakelock.

::: content/html/content/public/nsHTMLMediaElement.h
@@ +410,5 @@
>    class StreamListener;
>  
> +  class WakeLockBoolWrapper {
> +  public:
> +    WakeLockBoolWrapper(bool val = false) : mValue(val) {}

The WakeLockBoolWrapper ctor does not initialize mOuter, though I see the nsHTMLMediaElement ctor calls SetOuter(this) so this is not a fatal bug.

@@ +415,5 @@
> +    void SetOuter(nsHTMLMediaElement* outer) { mOuter = outer; }
> +    operator bool() { return mValue; }
> +    WakeLockBoolWrapper& operator=(bool val);
> +    bool operator !() { return !mValue; }
> +    bool operator !() const { return !mValue; }

The non-const overload of operator !() is redundant if you already have a const operator !().

@@ +919,5 @@
>    // sniffing phase, that would fail because sniffing only works when applied to
>    // the first bytes of the stream.
>    nsCString mMimeType;
> +
> +  // A wake lock this media element can hold to keep the screen on while playing

What is this comment commenting on?

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

chris, review ping?

Comment 16

[cajbir (:cajbir)]

Does this patch mean that any playing video, even if it's in a background tab, will prevent screen timeout? What if the browser is minimized (on desktop) or in the background on Fennec? If a video ad for example is playing in one of the tabs.

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Chris Double (:doublec) from comment #16)
> Does this patch mean that any playing video, even if it's in a background
> tab, will prevent screen timeout? What if the browser is minimized (on
> desktop) or in the background on Fennec? If a video ad for example is
> playing in one of the tabs.

In my mind that should all be handled by the wake lock implementation. If it isn't we should file a bug on it.

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ff8ce0a15867

Comment 19

[Cristian Nicolae (:xti)]

The device screen doesn't turn off anymore while the html5 video is playing on the latest Nightly. Closing bug as verified fixed on:

Firefox 19.0a1 (2012-11-16)
Device: Galaxy S2
OS: Android 4.0.3

Comment 20

[Ioana Chiorean]

A TC was added in MozTrap for this on 19 phones, 19 tablets, 20 phones, 20 tablets

https://moztrap.mozilla.org/manage/case/5944

Comment 21

[Justin Lebar (not reading bugmail)]

This code uses the wake lock API incorrectly.

The right thing to do here would be to acquire the "screen" wake lock, and only for <video> elements, not <audio> as well.  (As far as I can tell, this patch holds the screen on if the fg page is playing an <audio>.)

Then the code GeckoApp.java should check /specifically for the "screen" wake lock/, instead of holding the screen enabled whenever a page requests any wake lock.

Note that this lets any webpage acquire the "screen" wake lock and hold the screen on.  That may or may not be what you want to do.