616664 - display should not go to sleep while playing a video in fullscreen

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Watching a video is not pleasant when the display times put every 30 seconds. when the user is playing a video in fullscreen we can be reasonably sure that its their primary activity

Comment 1

[Madhava Enros [:madhava]]

Agreed. Watching a video entails involves not touching the screen for long periods of time, but that doesn't mean you're not using the device.

Comment 2

[Alon Zakai (:azakai)]

Looks pretty straightforward from the Android side,

http://developer.android.com/reference/android/os/PowerManager.html#SCREEN_BRIGHT_WAKE_LOCK

Question is though where to do this from our code. We have a |MakeFullScreen| that's called when entering and exiting fullscreen, but I am assuming we only want to disable the screensaver when (1) fullscreen for *video* (and not other content), and (2) the video is *playing* - is that correct?

Comment 3

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

IMO, this shouldn't be tied to MakeFullScreen(). I think your assumptions are roughly correct, but more generally this is a decision that should be pushed out the front end. So we should just provide a scriptable method to trigger it. nsIScreen::PreventTimeout()/AllowTimeout()? We'll have to play the interface frozen dance here regardless.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What if the user is just watching a Flash video on a site like Youtube, non-full-screen? Shouldn't we be keeping the screen awake for that too?

Comment 5

[Alon Zakai (:azakai)]

Attachment: patch

This is functional, so seems like a good time to check if the interfaces etc. are agreeable.

Overview:

* WakeLockWidget is an abstract base class that manages a list of wake locks. Widgets that support wake locks should inherit from it, and implement SetWakeLevel().
* SetWakeLevel() is called with the current wake lock level. That is, implementations don't need to mess with the concept of several wake locks existing - SetWakeLevel() is called with the current actual wake level.
* On Android, the implementation creates a wake lock of the proper level. That is, the OS also has a list of wake locks, which might seem unnecessary. However, other implementations might work in other ways (they might have a simple "disable screensaver" call, for example - that seems to be the case on GTK).

Comment 6

[Alon Zakai (:azakai)]

Attachment: WIP patch

mobile-browser portion. This currently will lock the screen when in fullscreen video mode, regardless of whether playing or not, so it isn't ready yet. Posting it now since the other patch might be easier to read with this alongside.

Comment 7

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

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

>@@ -1,9 +1,9 @@
>-/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-
>+/**
don't delete the mode line

>@@ -1,9 +1,9 @@
>-/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-
>+/**
no... really, don't delete my mode lines

>     }
> 
>     public static void hideProgressDialog() {
>         if (GeckoApp.mAppContext.mProgressDialog != null) {
>             GeckoApp.mAppContext.mProgressDialog.dismiss();
>             GeckoApp.mAppContext.mProgressDialog = null;
>         }
>     }
>+
>+    public static void acquireWakeLock(int type) {
>+        PowerManager pm = (PowerManager) GeckoApp.surfaceView.getContext().getSystemService(Context.POWER_SERVICE);
use GeckoApp.mAppContext not GeckoApp.surfaceView.getContext()

>+  long acquireWakeLock(in long type);

I think this maps too directly onto the android functionality. I hadn't thought of the dim versus turn off though. Perhaps enableScreenMode with normal/dimmable/no-sleep.  I'm not sure though, roc what do you think here?

I'm gonna r- for now, I'd like to hear what roc thinks.

Comment 8

[Alon Zakai (:azakai)]

Attachment: mobile-browser patch

Working mobile-browser patch - keeps screen on only during playing fullscreen video (not paused).

Not asking for review yet, since the wakelock API is in progress, but would be good to know if the mobile-browser bits are generally correct.

Comment 9

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

Comment on attachment 498274 [details] [diff] [review]
mobile-browser patch


>   init: function fsv_init() {
>     messageManager.addMessageListener("Browser:FullScreenVideo:Start", this.show.bind(this));
>     messageManager.addMessageListener("Browser:FullScreenVideo:Close", this.hide.bind(this));
>+    messageManager.addMessageListener("Browser:FullScreenVideo:Play", (function() {
  <snip>
>+    }).bind(this));
>+    messageManager.addMessageListener("Browser:FullScreenVideo:Pause", (function() {
  <snip>
>+    }).bind(this));

Should probably use real methods here, like this.show() and this.hide()

>diff --git a/chrome/content/fullscreen-video.js b/chrome/content/fullscreen-video.js

>-function closeFullScreen() {
>-  sendAsyncMessage("Browser:FullScreenVideo:Close");
>-}
>-
> addEventListener("click", function(aEvent) {
>   if (aEvent.target.id == "close")
>     closeFullScreen();
> }, false);

closeFullScreen() is still called here

Approach looks good to me

Comment 10

[Michael Wu [:mwu]]

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

Please check if http://developer.android.com/reference/android/view/View.html#setKeepScreenOn%28boolean%29 will be sufficient for our needs.

Comment 11

[Alon Zakai (:azakai)]

(In reply to comment #10)
> Comment on attachment 498261 [details] [diff] [review]
> patch
> 
> Please check if
> http://developer.android.com/reference/android/view/View.html#setKeepScreenOn%28boolean%29
> will be sufficient for our needs.

Nice find, thanks!

However it isn't clear to me from the docs whether this keeps the screen brightly lit or dimmed. So I prefer the current patch, which uses the Android API that explicitly keeps it bright.

In any case the difference is just a few lines of Java code, so there isn't much cost to doing things as the patch does.

Comment 12

[Alon Zakai (:azakai)]

Just to further clarify the API in the current patch:

1. Internally, our UI scripts (and potentially other code, of course) use an acquireWakeLock/releaseWakeLock API. This API happens to look similar to the one in Android, but the only reason it does is that I think that kind of API is the minimal one that is sane and provides all the functionality (namely, it allows multiple components to acquire separate wake locks).

2. Each widget implementation just needs to implement a SetWakeLevel() function. In other words each widget code needs just to implement something much simpler than the API mentioned in 1.

3. In our Android widget implementation, we receive SetWakeLevel() and then call into Android's wake lock API, acquiring a single lock of the right sort. Again, this is similar to the API in 1, but this is just superficially. Other widget code might be very different (e.g. I believe that GTK's would use the DBUS screensaver stuff, which has no concept of multiple locks AFAICT).

Summary: So, in our UI code we acquire locks as we need them. There can be any number of them. The widget-independent code manages those locks and sends out to widget-specific code a simple, single request for the current wake level.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Seems to me that the minimal API here would be something like (on nsIScreen):
  const long BRIGHTNESS_DIM = 1;
  const long BRIGHTNESS_FULL = 2;
  void lockMinimumBrightness(long brightness);
  void unlockMinimumBrightness(long brightness);

where every call to lockMinimumBrightness(b) should be followed by a call to unlockMinimumBrightness(b) (eventually).

Sound OK?

Comment 14

[Alon Zakai (:azakai)]

Attachment: patch v2

I like that API very much. Here's a patch with it.

Comment 15

[Alon Zakai (:azakai)]

Attachment: android bits

Android bits, separated out for reviewing convenience.

Comment 16

[Alon Zakai (:azakai)]

Attachment: mobile-browser patch v2

Updated mobile-browser patch.

Comment 17

[Michael Wu [:mwu]]

Comment on attachment 498883 [details] [diff] [review]
android bits

>diff --git a/embedding/android/GeckoAppShell.java b/embedding/android/GeckoAppShell.java
>--- a/embedding/android/GeckoAppShell.java
>+++ b/embedding/android/GeckoAppShell.java
>@@ -37,16 +37,17 @@
> 
> package org.mozilla.gecko;
> 
> import java.io.*;
> import java.util.*;
> import java.util.zip.*;
> import java.nio.*;
> import java.lang.reflect.*;
>+import java.lang.Runnable;
IIRC java.lang.*; is autoimported for you.

>@@ -633,9 +639,21 @@ class GeckoAppShell
>     }
> 
>     public static void hideProgressDialog() {
>         if (GeckoApp.mAppContext.mProgressDialog != null) {
>             GeckoApp.mAppContext.mProgressDialog.dismiss();
>             GeckoApp.mAppContext.mProgressDialog = null;
>         }
>     }
>+
>+    public static void setKeepScreenOn(final boolean on) {
>+        class DoIt
>+            implements Runnable
>+        {
>+            public void run() {
>+                GeckoApp.surfaceView.setKeepScreenOn(on);
>+            }
>+        }
>+        sTheHandler.post(new DoIt());

Use http://developer.android.com/reference/android/app/Activity.html#runOnUiThread%28java.lang.Runnable%29 and use an anonymous inner class for the Runnable.

>diff --git a/widget/src/android/AndroidBridge.h b/widget/src/android/AndroidBridge.h
>--- a/widget/src/android/AndroidBridge.h
>+++ b/widget/src/android/AndroidBridge.h
>@@ -206,16 +206,18 @@ public:
>         int mEntries;
>     };
> 
>     /* See GLHelpers.java as to why this is needed */
>     void *CallEglCreateWindowSurface(void *dpy, void *config, AndroidGeckoSurfaceView& surfaceView);
> 
>     bool GetStaticStringField(const char *classID, const char *field, nsAString &result);
> 
>+    void SetKeepScreenOn(PRBool on);
>+
bool on

>diff --git a/widget/src/android/nsScreenManagerAndroid.cpp b/widget/src/android/nsScreenManagerAndroid.cpp
>--- a/widget/src/android/nsScreenManagerAndroid.cpp
>+++ b/widget/src/android/nsScreenManagerAndroid.cpp
>@@ -33,18 +33,22 @@
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsScreenManagerAndroid.h"
> #include "nsWindow.h"
>+#include "AndroidJavaWrappers.h"

What's this include for?

>+#include "AndroidBridge.h"
> 
>-NS_IMPL_ISUPPORTS1(nsScreenAndroid, nsIScreen)
>+using namespace mozilla;
>+
>+NS_IMPL_ISUPPORTS2(nsScreenAndroid, nsIScreen, nsIScreen_MOZILLA_2_0_BRANCH)
> 
> nsScreenAndroid::nsScreenAndroid(void *nativeScreen)
> {
> }
> 
> nsScreenAndroid::~nsScreenAndroid()
> {
> }
>@@ -83,16 +87,24 @@ nsScreenAndroid::GetPixelDepth(PRInt32 *
> 
> 
> NS_IMETHODIMP
> nsScreenAndroid::GetColorDepth(PRInt32 *aColorDepth)
> {
>     return GetPixelDepth(aColorDepth);
> }
> 
>+void
>+nsScreenAndroid::ApplyMinimumBrightness(PRUint32 aType)
>+{
>+  AndroidBridge::Bridge()->SetKeepScreenOn(aType == BRIGHTNESS_FULL);
>+}
>+
>+//
>+

remove the // and extra blank line.

Comment 18

[Alon Zakai (:azakai)]

Attachment: android bits v2

Fixed android bits.

Comment 19

[Michael Wu [:mwu]]

Comment on attachment 498910 [details] [diff] [review]
android bits v2

>diff --git a/embedding/android/GeckoAppShell.java b/embedding/android/GeckoAppShell.java
>--- a/embedding/android/GeckoAppShell.java
>+++ b/embedding/android/GeckoAppShell.java
>@@ -1,9 +1,9 @@
>-/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-
>+/* -*- Mode: Java; c-basic-offset: 4; tab-width: 4; indent-tabs-mode: nil; -*-

If blassey added this modeline, he probably doesn't want you messing with it. I have no opinion here.

>diff --git a/widget/src/android/nsScreenManagerAndroid.h b/widget/src/android/nsScreenManagerAndroid.h
>--- a/widget/src/android/nsScreenManagerAndroid.h
>+++ b/widget/src/android/nsScreenManagerAndroid.h
>-class nsScreenAndroid :
>-    public nsIScreen
>+class nsScreenAndroid
>+  : public nsIScreen
>+  , public mozilla::widget::BrightnessLockingWidget

This part seems a little strange to me. Is BrightnessLockingWidget basically a base class for nsIScreen implementations that want to use brightness locking? If that's the case, it seems like base classes are usually implemented in files named nsBase* in xpwidgets and it might be better to follow that pattern. I'm not reviewing that part though, so r=me regardless.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 498881 [details] [diff] [review]
patch v2

We don't need BRIGHTNESS_OFF, right? It doesn't really make sense to set a minimum brightness of OFF, surely? :-)

+  virtual void ApplyMinimumBrightness(PRUint32 aType) = 0;

Can't this just be protected?

Comment 21

[Alon Zakai (:azakai)]

Attachment: patch v3

(In reply to comment #19)
> Comment on attachment 498910 [details] [diff] [review]
> android bits v2
> 
> >diff --git a/embedding/android/GeckoAppShell.java b/embedding/android/GeckoAppShell.java
> >--- a/embedding/android/GeckoAppShell.java
> >+++ b/embedding/android/GeckoAppShell.java
> >@@ -1,9 +1,9 @@
> >-/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-
> >+/* -*- Mode: Java; c-basic-offset: 4; tab-width: 4; indent-tabs-mode: nil; -*-
> 
> If blassey added this modeline, he probably doesn't want you messing with it. I
> have no opinion here.

blassey, I just changed the tab-width, following our chat last week. Is that ok?

(In reply to comment #20)
> Comment on attachment 498881 [details] [diff] [review]
> patch v2
> 
> We don't need BRIGHTNESS_OFF, right? It doesn't really make sense to set a
> minimum brightness of OFF, surely? :-)

I was using BRIGHTNESS_OFF internally for convenience. You are totally right, that has no business being in an interface. Removed it.

> 
> +  virtual void ApplyMinimumBrightness(PRUint32 aType) = 0;
> 
> Can't this just be protected?

Yes, very true. Changed.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 498932 [details] [diff] [review]
patch v3

sr=me, maybe Brad can review it?

Comment 24

[Alon Zakai (:azakai)]

m-c parts:

http://hg.mozilla.org/mozilla-central/rev/d488f09f6f8d
http://hg.mozilla.org/mozilla-central/rev/77956e4dd8f8

Comment 25

[Alon Zakai (:azakai)]

m-b part:

http://hg.mozilla.org/mobile-browser/rev/40f2c3829452

Comment 26

[Tony Chung [:tchung]]

need to verify this test.  Alon, do you have automated tests for this?

Comment 27

[Alon Zakai (:azakai)]

There is no practical way to automatically test this that I can see - one needs to actually start a fullscreen video and wait a while to see that the screen doesn't dim. An automatic test can't detect the screen dimming.

Comment 28

[Benjamin Smedberg]

I tried to remove the _MOZILLA_2_0 interface added here post-2.0, but it seems that not every screen implementation implements the new interface, only the android implementation does. If so, should this really be a unified interface? Or should this be nsIScreenBrightness and left as a separate interface?

Comment 29

[Alon Zakai (:azakai)]

I don't have an opinion about unified or separate, but I do think this would be useful in other places than Android. It would be nice to not have the display go to sleep on desktop too (like Flash video).

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we should make everything implement the new interface, with a dummy implementation for non-Android.

Comment 31

[Andreea Pod]

VERIFIED FIXED on:
Build ID: Mozilla /5.0 (Android;Linux armv7l;rv:2.0b13pre) Gecko/20110315
Firefox/4.0b13pre Fennec /4.0b6pre
Device: HTC Desire

Comment 32

[Ioana Chiorean]

https://litmus.mozilla.org/show_test.cgi?id=15202