Bug 227047 - [iOS] Fullscreen video playback gets stuck after interacting with the playback controls
Summary: [iOS] Fullscreen video playback gets stuck after interacting with the playbac...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Peng Liu
URL:
Keywords: InRadar
: 227106 (view as bug list)
Depends on:
Blocks: 227106
  Show dependency treegraph
 
Reported: 2021-06-15 14:37 PDT by Peng Liu
Modified: 2021-06-21 15:35 PDT (History)
8 users (show)

See Also:


Attachments
Patch (37.67 KB, patch)
2021-06-15 15:19 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Patch (37.66 KB, patch)
2021-06-15 15:24 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Fix build failures on macOS (38.85 KB, patch)
2021-06-15 16:31 PDT, Peng Liu
eric.carlson: review+
Details | Formatted Diff | Diff
Revise the patch based on Devin and Eric's comments (42.34 KB, patch)
2021-06-17 15:11 PDT, Peng Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peng Liu 2021-06-15 14:37:54 PDT
[iOS] Fullscreen video playback gets stuck after interacting with the playback controls
Comment 1 Peng Liu 2021-06-15 14:38:45 PDT
<rdar://79103461>
Comment 2 Peng Liu 2021-06-15 15:19:37 PDT
Created attachment 431486 [details]
Patch
Comment 3 Peng Liu 2021-06-15 15:24:38 PDT
Created attachment 431487 [details]
Patch
Comment 4 Devin Rousso 2021-06-15 15:50:45 PDT
Comment on attachment 431486 [details]
Patch

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

> Source/WebCore/platform/cocoa/PlaybackSessionModel.h:105
> +    virtual void rateChanged(bool /* isPlaying */, bool /* isStalled */, double /* playbackRate */, double /* defaultPlaybackRate */) { }

Could we maybe have an `OptionSet` of enum flags?

> Source/WebCore/platform/cocoa/PlaybackSessionModelMediaElement.mm:-55
> -static const float StalledPlaybackRate = 0.00000001f;

Do we still need to report this value (or something like it) to AVKit?  Or will AVKit be fine with a playback rate of 1 (or 2 or 0.5 or whatever) when we're still loading?

> Source/WebCore/platform/ios/PlaybackSessionInterfaceAVKit.mm:130
> +    if (!isStalled)

I wonder if instead of not modifying `m_playerController` we should instead have a `stalled` property on `WebAVPlayerController` and have `setRate:` ignore/defer modifications if `stalled`.  This way, if AVKit calls `setRate:` then we will wait to update the rate until we're no longer stalled (e.g. inside `setStalled:` you could send the `rate` back to the `model`).  Though I think all of this assumes my comment on PlaybackSessionModelMediaElement.mm:55 is accurate.
Comment 5 Peng Liu 2021-06-15 16:31:05 PDT
Created attachment 431494 [details]
Fix build failures on macOS
Comment 6 Peng Liu 2021-06-16 15:15:52 PDT
Comment on attachment 431486 [details]
Patch

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

>> Source/WebCore/platform/cocoa/PlaybackSessionModel.h:105
>> +    virtual void rateChanged(bool /* isPlaying */, bool /* isStalled */, double /* playbackRate */, double /* defaultPlaybackRate */) { }
> 
> Could we maybe have an `OptionSet` of enum flags?

Yes. Like an `OptionSet` of
```
enum class PlaybackState {
    Playing = 1 << 0,
    Stalled = 1 << 1,
};
```

>> Source/WebCore/platform/cocoa/PlaybackSessionModelMediaElement.mm:-55
>> -static const float StalledPlaybackRate = 0.00000001f;
> 
> Do we still need to report this value (or something like it) to AVKit?  Or will AVKit be fine with a playback rate of 1 (or 2 or 0.5 or whatever) when we're still loading?

With this patch, I cannot reproduce bug 211797. So looks like AVKit does not need this value anymore.

>> Source/WebCore/platform/ios/PlaybackSessionInterfaceAVKit.mm:130
>> +    if (!isStalled)
> 
> I wonder if instead of not modifying `m_playerController` we should instead have a `stalled` property on `WebAVPlayerController` and have `setRate:` ignore/defer modifications if `stalled`.  This way, if AVKit calls `setRate:` then we will wait to update the rate until we're no longer stalled (e.g. inside `setStalled:` you could send the `rate` back to the `model`).  Though I think all of this assumes my comment on PlaybackSessionModelMediaElement.mm:55 is accurate.

Hmm, this approach sounds safer. However, if `m_playerController` does not report the rate back to the WebContent process when its `stalled` property is YES, its rate might be overwritten by a value comes from the WebContent process. That could happen when a user changes playback rate in fullscreen immediately after scrubbing the video.
Comment 7 Eric Carlson 2021-06-17 07:44:10 PDT
Comment on attachment 431494 [details]
Fix build failures on macOS

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

> Source/WebCore/platform/cocoa/PlaybackSessionModel.h:105
> +    virtual void rateChanged(bool /* isPlaying */, bool /* isStalled */, double /* playbackRate */, double /* defaultPlaybackRate */) { }

I agree that an enums and an OptionSet would be nicer than bools
Comment 8 Peng Liu 2021-06-17 15:11:36 PDT
Created attachment 431727 [details]
Revise the patch based on Devin and Eric's comments
Comment 9 EWS 2021-06-18 12:15:48 PDT
Committed r279043 (238963@main): <https://commits.webkit.org/238963@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 431727 [details].
Comment 10 Peng Liu 2021-06-21 15:35:58 PDT
*** Bug 227106 has been marked as a duplicate of this bug. ***