Bug 147269 – [iOS] REGRESSION(r168075): Fullscreen web video doesn't pause on screen lock

• Component: Media
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

When a video is playing in full screen mode and the device is locked, the video continues playing. PiP (picture in picture) mode overrides the EnterBackground notification and just ignores it and does not suspend the video playing.

Media elements should pause when the application is going to EnterBackground under lock regardless whether it is in full screen or not.

[Said Abou-Hallawa]

<rdar://problem/19892014>

[Said Abou-Hallawa]

Created attachment 257467 [details]
Patch

[Said Abou-Hallawa]

Created attachment 257470 [details]
Patch

[Said Abou-Hallawa]

Created attachment 257473 [details]
Patch

[Jer Noble]

Comment on attachment 257473 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257473&action=review

> Source/WebCore/platform/audio/PlatformMediaSession.cpp:99
> +void PlatformMediaSession::endSuspend(EndInterruptionFlags flags)
> +{
> +    State stateToRestore = m_stateToRestore;
> +    m_stateToRestore = Idle;
> +    setState(Paused);
> +
> +    bool shouldResume = flags & MayResumePlaying && stateToRestore == Playing;
> +    client().mayResumePlayback(shouldResume);
> +}

This refactoring looks unnecessary, as endSuspend() isn't called from anywhere other than endInterruption().

> Source/WebCore/platform/audio/PlatformMediaSession.cpp:121
> +    if (!m_interruptionCount) {
> +        ASSERT(false);

This ASSERT(false) is weird.  I think you may want a ASSERT_NOT_REACHED().

> Source/WebCore/platform/audio/PlatformMediaSession.cpp:122
> +        ++m_interruptionCount;

So, if m_interruptionCount == 0, m_interruptionCount will not increment?  Won't this lead to a mismatch when endInterruption() is called?

> Source/WebCore/platform/audio/PlatformMediaSession.cpp:124
> +    } else if (shouldInterrupt(type))
> +        return;

Wait, so if we shouldInterrupt(type), we return early without calling beginSuspend()?

All this is super confusing, and not obvious from the written logic. If this is correct, we're going to need to add some explanatory comments, or re-write the logic so it's less confusing.

[Said Abou-Hallawa]

Comment on attachment 257473 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257473&action=review

>> Source/WebCore/platform/audio/PlatformMediaSession.cpp:121
>> +        ASSERT(false);
> 
> This ASSERT(false) is weird.  I think you may want a ASSERT_NOT_REACHED().

ASSERT_NOT_REACHED() is used instead of ASSERT(false).

>> Source/WebCore/platform/audio/PlatformMediaSession.cpp:122
>> +        ++m_interruptionCount;
> 
> So, if m_interruptionCount == 0, m_interruptionCount will not increment?  Won't this lead to a mismatch when endInterruption() is called?

Logic is simplified. beginInterruption() has to be called before calling forceInterruption(). forceInterruption() will not increment m_interruptionCount. And if m_interruptionCount is zero, forceInterruption() will assert and return.

>> Source/WebCore/platform/audio/PlatformMediaSession.cpp:124
>> +        return;
> 
> Wait, so if we shouldInterrupt(type), we return early without calling beginSuspend()?
> 
> All this is super confusing, and not obvious from the written logic. If this is correct, we're going to need to add some explanatory comments, or re-write the logic so it's less confusing.

I am going to add a comment before this if-statemnet saying: "The purpose of this function is to override the decision which was made by beginInterruption(). If it was decided to interrupt the media session there, nothing should be done here.

[Said Abou-Hallawa]

Created attachment 257601 [details]
Patch

[Said Abou-Hallawa]

Unfortunately this patch does not fix the PiP case with locking the device. When the HTML5 video element is playing in full screen mode and the Home button is pressed, the media control goes into PiP mode. This is the case where media control is minimized and the Springboard is the foreground application. If the device is locked, MobileSafari application does not receive any notification for being deactivated. It seems it is a UIKit issue and I am not sure if it should be addressed separately or addressed in this patch.

[Said Abou-Hallawa]

Comment on attachment 257601 [details]
Patch

Make this patch WIP till I figure out the solution for the PiP locking case.

[Said Abou-Hallawa]

Comment on attachment 257601 [details]
Patch

Since the time for iOS 9 is now limited, let's have this one reviewed while finding an incremental fix for the Picture-in-Picture case.

[Eric Carlson]

Comment on attachment 257601 [details]
Patch

This looks fine to me, but I am not a WK2 reviewer so someone else will have to give the official r+

[Jon Lee]

Re-setting the r?

[Jon Lee]

Comment on attachment 257601 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257601&action=review

> Source/WebCore/ChangeLog:19
> +        (WebCore::PlatformMediaSession::shouldInterrupt): Move the condition 

Can we update this Changelog to match the new names of the functions?

[Jer Noble]

Comment on attachment 257601 [details]
Patch

I also give this an unofficial r+. (Unofficial due to the WebKit2 changes.)

[Said Abou-Hallawa]

Created attachment 257680 [details]
Patch

[Said Abou-Hallawa]

Change(In reply to comment #13)
> Comment on attachment 257601 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257601&action=review
> 
> > Source/WebCore/ChangeLog:19
> > +        (WebCore::PlatformMediaSession::shouldInterrupt): Move the condition 
> 
> Can we update this Changelog to match the new names of the functions?

Done.

[Andreas Kling]

Comment on attachment 257680 [details]
Patch

r=me as WK2 owner given Eric and Jer's approval.

[WebKit Commit Bot]

Comment on attachment 257680 [details]
Patch

Clearing flags on attachment: 257680

Committed r187522: <http://trac.webkit.org/changeset/187522>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.