868325 - VideoElement should require a 'screen' wakeLock when active

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andrea Marchesini [:baku]]

Follow up for bug 859824 Comment 63

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 2

[Justin Lebar (not reading bugmail)]

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

>+   * These 2 methods are called by the WakeLockBoolWrapper when the wakelock

s/2/two

(We don't usually use Arabic numerals for numbers less than five or ten when we're counting things.)

But more importantly than this, I think we want to hold the screen wake lock only if the <video> is in an fg document.

The reason it's different from the <audio> case is that, no matter where on a page a user is, and no matter whether the <audio> is in a foreground or background tab, the user can hear an <audio> if it's playing.

In contrast, silent <video>s are not uncommon, and if the <video> is not in the foreground, you don't want to keep the screen on.  In fact, we probably shouldn't hold the CPU lock for videos playing in the background, either.

You can add a visibilitychange listener to the video's document to find out when it's visible and not.  Note that I think it's "mozvisibilitychange" in b2g18 and "visibilitychange" on trunk.  grep will tell you.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 4

[Justin Lebar (not reading bugmail)]

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

>+void nsHTMLVideoElement::NotifyOwnerDocumentActivityChanged()
>+{
>+  nsHTMLMediaElement::NotifyOwnerDocumentActivityChanged();
>+  WakeLockUpdate();
>+}
>+
>+void
>+nsHTMLVideoElement::WakeLockCreate()
>+{
>+  nsHTMLMediaElement::WakeLockCreate();
>+  mWakeLockCreated = true;
>+  WakeLockUpdate();
>+}
>+
>+void
>+nsHTMLVideoElement::WakeLockRelease()
>+{
>+  nsHTMLMediaElement::WakeLockRelease();
>+  mWakeLockCreated = false;
>+  WakeLockUpdate();
>+}
>+
>+void
>+nsHTMLVideoElement::WakeLockUpdate()
>+{
>+  bool hidden = false;

Hidden should probably default to true, although I'm pretty sure OwnerDoc is
never null.  (Otherwise it would be called GetOwnerDoc().)

I don't think we need to hold the CPU wake lock for <video>'s if we're holding
the screen wake lock, so I think we can avoid calling
nsHTMLMediaElement::WakeLockRelease() above.  We certainly don't want to hold
the CPU wake lock if the screen is off.

>+  nsCOMPtr<nsIDOMDocument> domDoc = do_QueryInterface(OwnerDoc());
>+  if (domDoc) {
>+    domDoc->GetHidden(&hidden);
>+  }
>+
>+  if (mScreenWakeLock && (!mWakeLockCreated || hidden)) {

mWakeLockCreated isn't the right name; this variable actually tells us whether
we are playing.  Can we instead read nsHTMLMediaElement::mPaused somehow
(through a function or whatever)?

>diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp
>--- a/dom/ipc/ContentParent.cpp
>+++ b/dom/ipc/ContentParent.cpp

> bool
> ContentParent::GetParamsForBlob(nsDOMFileBase* aBlob,
>                                 BlobConstructorParams* aOutParams)
> {
>   BlobConstructorParams resultParams;
> 
>-  if (!(aBlob->IsMemoryBacked() || aBlob->GetSubBlobs()) &&
>-      (aBlob->IsSizeUnknown() || aBlob->IsDateUnknown())) {
>-    // We don't want to call GetSize or GetLastModifiedDate
>-    // yet since that may stat a file on the main thread
>-    // here. Instead we'll learn the size lazily from the
>-    // other process.
>-    resultParams = MysteryBlobConstructorParams();
>-  }
>-  else {

Did you mean to include this change?  If so, I don't get it.

I don't know if you're into testing this stuff, but it's not too hard, and it's a good feeling to know that we won't regress it.  :)

Comment 5

[Andrea Marchesini [:baku]]

Attachment: patch

Yeah... all of the patch was an experiment for reproducing another bug.

Comment 6

[Justin Lebar (not reading bugmail)]

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

>+void
>+nsHTMLVideoElement::WakeLockUpdate()
>+{
>+  bool hidden = true;
>+
>+  nsCOMPtr<nsIDOMDocument> domDoc = do_QueryInterface(OwnerDoc());
>+  if (domDoc) {
>+    domDoc->GetHidden(&hidden);
>+  }
>+
>+  bool paused = true;
>+  GetPaused(&paused);
>+
>+  if (mScreenWakeLock && (mPaused || hidden)) {

paused instead of mPaused?  If mPaused works, then perhaps you don't need the
call to GetPaused.

Also, is GetPaused true when we're paused by the audio channel?

r=me if the above checks out.  I still think it's worth considering writing a
unit test for this, but I'm not going to gate the review on it.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: patch

It's always a good idea to write tests. Here we have a test, and this test shows that:

mScreenWakeLock = nullptr;

is not enough to unlock it. It seems that a reference is taken by something else.
I suggest to:

1. Unlock the audio wakelock too - because that patch is landed.
2. Apply the patch for gaia.
3. Investigate why this wakelock is not unlocked when it set to nullptr in a follow up.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: gaia patch

https://github.com/mozilla-b2g/gaia/pull/9569

Comment 9

[Andrea Marchesini [:baku]]

Ok, it seems that AddSystemEventListener() increases the refcounter of the wakelock, so wakelock = null is not enough.

Comment 10

[Justin Lebar (not reading bugmail)]

(In reply to Andrea Marchesini (:baku) from comment #9)
> Ok, it seems that AddSystemEventListener() increases the refcounter of the
> wakelock, so wakelock = null is not enough.

Where is this being called on the wake lock (or on an object which holds a ref to the wake lock)?

Unlocking the wake lock is fine (good, even), but this also implies that we leak wake locks for some period of time.

Comment 11

[Justin Lebar (not reading bugmail)]

> 1. Unlock the audio wakelock too - because that patch is landed.

Can you do that at least in a separate patch?  It's a follow-up to a tef bug, while this bug has no flags.

Comment 12

[Justin Lebar (not reading bugmail)]

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

Did you make any changes to the source code that you want me to look at here?

We don't write tests which use setTimeout like this.  The reason is that they create randomorange.

The right way to do this is to call SimpleTest.finish() after you get the two WakeLock events.  You could add a delay before calling Finish to ensure that we don't get any other WakeLock events in the meantime, but that's not necessary.

It would probably be helpful if we tested that the wake lock gets released when the video finishes playing on its own.  We also might as well write a test for the audio bits, along with that patch.  I have a silence.ogg sitting in the tree somewhere, although I'd appreciate if you copied it instead of referencing it (it's small).

Comment 13

[Justin Lebar (not reading bugmail)]

Comment on attachment 745789 [details] [diff] [review]
gaia patch

r-; see PR.

https://github.com/mozilla-b2g/gaia/pull/9569

Comment 14

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 15

[Justin Lebar (not reading bugmail)]

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

Does the video have to be so big?  Surely if we had a small, black square as the video, it would be a smaller file...

The same bit about silence.ogg applies here: We probably want a long and a short video for the test.

Also, this is going to cause us to leak a WakeLock every time we call WakeLockRelease(), so I think we want to do that bit about unregistering wake lock listeners when it's unlocked.

Comment 16

[Andrea Marchesini [:baku]]

> Also, this is going to cause us to leak a WakeLock every time we call
> WakeLockRelease(), so I think we want to do that bit about unregistering
> wake lock listeners when it's unlocked.

This already happens in WakeLock::Unlock(). Here we do: 

  DoUnlock();
  DetachEventListener();

Comment 17

[Andrea Marchesini [:baku]]

Attachment: b2g18 patch

Comment 18

[Andrea Marchesini [:baku]]

https://github.com/mozilla-b2g/gaia/pull/9584

I don't think we should remove the wakelock in the camera app.

Comment 19

[Justin Lebar (not reading bugmail)]

> This already happens in WakeLock::Unlock(). Here we do: 
>
>  DoUnlock();
>  DetachEventListener();

Indeed, my mistake.

Comment 20

[Andrea Marchesini [:baku]]

Attachment: m-c patch

Comment 21

[Andrea Marchesini [:baku]]

At least we can land the m-c patch. I would like to see this patch landed for v1.1.0.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/a5fd9bdc9bd8

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

[Justin Lebar (not reading bugmail)]

Comment on attachment 745789 [details] [diff] [review]
gaia patch

Sorry for taking a while to get to this one.

r=me if you've tested this in at least one of these apps.

Comment 25

[Andrea Marchesini [:baku]]

Where can we land this gaia patch? Can someone take care of landing it... ?

Comment 26

[Justin Lebar (not reading bugmail)]

Just make a pull request and I can merge it.

Comment 27

[Andrea Marchesini [:baku]]

https://github.com/mozilla-b2g/gaia/pull/9584

I mark you as need-info, just because I don't know to mark this bug for been landing.

Comment 28

[Andrea Marchesini [:baku]]

https://github.com/mozilla-b2g/gaia/pull/9858  new pull request.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/1fdd57d23be0

The Gaia parts will still need uplifting. John, can you please assist with that?

Comment 30

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #29)
> https://hg.mozilla.org/releases/mozilla-b2g18/rev/1fdd57d23be0
> 
> The Gaia parts will still need uplifting. John, can you please assist with
> that?

Has the gaia patch landed?  I can't find a commit in this bug or the pull requests and I couldn't find the bug number in the master branch commit log.

Comment 31

[Justin Lebar (not reading bugmail)]

If it's https://github.com/mozilla-b2g/gaia/pull/9858, I think it hasn't landed...

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Indeed, that looks right. Looks like this'll need master and v1-train love then :)

Comment 33

[Andrea Marchesini [:baku]]

Yes, that was the patch. Here a new one with love :)

https://github.com/mozilla-b2g/gaia/pull/10059

Comment 34

[Justin Lebar (not reading bugmail)]

Merged.  https://github.com/mozilla-b2g/gaia/commit/2970582d0ed5f5514090fa53429c2ed5db6eea1d

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Still needs to land on v1-train.

Comment 36

[John Ford [:jhford] CET/CEST Berlin Time]

v1-train: 2f10d28