Bug 147269 - [iOS] REGRESSION(r168075): Fullscreen web video doesn't pause on screen lock
Summary: [iOS] REGRESSION(r168075): Fullscreen web video doesn't pause on screen lock
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2015-07-24 11:46 PDT by Said Abou-Hallawa
Modified: 2015-07-28 15:31 PDT (History)
8 users (show)

See Also:


Attachments
Patch (17.34 KB, patch)
2015-07-24 12:19 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (17.09 KB, patch)
2015-07-24 12:50 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (17.61 KB, patch)
2015-07-24 13:34 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (17.27 KB, patch)
2015-07-27 15:30 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (17.07 KB, patch)
2015-07-28 14:05 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2015-07-24 11:46:54 PDT
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.
Comment 1 Said Abou-Hallawa 2015-07-24 11:47:45 PDT
<rdar://problem/19892014>
Comment 2 Said Abou-Hallawa 2015-07-24 12:19:28 PDT
Created attachment 257467 [details]
Patch
Comment 3 Said Abou-Hallawa 2015-07-24 12:50:30 PDT
Created attachment 257470 [details]
Patch
Comment 4 Said Abou-Hallawa 2015-07-24 13:34:15 PDT
Created attachment 257473 [details]
Patch
Comment 5 Jer Noble 2015-07-27 13:12:58 PDT
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.
Comment 6 Said Abou-Hallawa 2015-07-27 15:30:11 PDT
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.
Comment 7 Said Abou-Hallawa 2015-07-27 15:30:27 PDT
Created attachment 257601 [details]
Patch
Comment 8 Said Abou-Hallawa 2015-07-27 15:36:37 PDT
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.
Comment 9 Said Abou-Hallawa 2015-07-27 15:54:18 PDT
Comment on attachment 257601 [details]
Patch

Make this patch WIP till I figure out the solution for the PiP locking case.
Comment 10 Said Abou-Hallawa 2015-07-28 08:41:27 PDT
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.
Comment 11 Eric Carlson 2015-07-28 13:07:27 PDT
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+
Comment 12 Jon Lee 2015-07-28 13:39:29 PDT
Re-setting the r?
Comment 13 Jon Lee 2015-07-28 13:49:12 PDT
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?
Comment 14 Jer Noble 2015-07-28 13:58:02 PDT
Comment on attachment 257601 [details]
Patch

I also give this an unofficial r+. (Unofficial due to the WebKit2 changes.)
Comment 15 Said Abou-Hallawa 2015-07-28 14:05:23 PDT
Created attachment 257680 [details]
Patch
Comment 16 Said Abou-Hallawa 2015-07-28 14:09:15 PDT
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.
Comment 17 Andreas Kling 2015-07-28 14:35:20 PDT
Comment on attachment 257680 [details]
Patch

r=me as WK2 owner given Eric and Jer's approval.
Comment 18 WebKit Commit Bot 2015-07-28 15:31:21 PDT
Comment on attachment 257680 [details]
Patch

Clearing flags on attachment: 257680

Committed r187522: <http://trac.webkit.org/changeset/187522>
Comment 19 WebKit Commit Bot 2015-07-28 15:31:26 PDT
All reviewed patches have been landed.  Closing bug.