Bug 216858 – [iOS] unable to airplay directly loaded fullscreen video

• Component: Media
• Status: RESOLVED FIXED

[Devin Rousso]

By "directly loaded" I mean where the main resource is a video file (e.g. URL ends in `.mp4`).  This issue doesn't appear to reproduce on pages that load videos (e.g. HTML pages that have `<video src="...">`).

[Devin Rousso]

<rdar://problem/68746321>

[Devin Rousso]

Created attachment 409426 [details]
Patch

This doesn't have any tests yet.  I'm mainly uploading it to see what the bots think while I figure out how to test this (if it's even possible, as it only appears to reproduce on directly loaded videos).

[Peng Liu]

Comment on attachment 409426 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +        is initally reported as "disabled" and then almost immediately changed to "enabled".

Sounds like AVKit does not use the up-to-date value of the variable?

[Devin Rousso]

Created attachment 409518 [details]
Patch

Re-approached the fix from another angle.  Working on writing tests.

[Devin Rousso]

Comment on attachment 409426 [details]
Patch

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

>> Source/WebCore/ChangeLog:18
>> +        is initally reported as "disabled" and then almost immediately changed to "enabled".
> 
> Sounds like AVKit does not use the up-to-date value of the variable?

as it turns out, it's more like "WebKit did not propagate the new value if/when the value returned by `wirelessVideoPlaybackDisabled` changes to the UIProcess so that WebKit could call `-[WebAVPlayerController setAllowsExternalPlayback:]` again with the new value" :P

please see attachment 409518 [details] for more details :)

[Peng Liu]

Comment on attachment 409518 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:5009
> +    if (m_player) {

Is this change needed?

[Devin Rousso]

Comment on attachment 409518 [details]
Patch

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

>> Source/WebCore/html/HTMLMediaElement.cpp:5009
>> +    if (m_player) {
> 
> Is this change needed?

yes, as otherwise this function early-returns before the added code below is able to run

I've tried looking at whether any of the other functions after this do in-fact require a `m_player` to exist, and I don't think they do, but this code is quite nested and intertwined so I'm not entirely sure.  I figured that EWS would tell me if this was bad :)

[Peng Liu]

Comment on attachment 409518 [details]
Patch

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

>>> Source/WebCore/html/HTMLMediaElement.cpp:5009
>>> +    if (m_player) {
>> 
>> Is this change needed?
> 
> yes, as otherwise this function early-returns before the added code below is able to run
> 
> I've tried looking at whether any of the other functions after this do in-fact require a `m_player` to exist, and I don't think they do, but this code is quite nested and intertwined so I'm not entirely sure.  I figured that EWS would tell me if this was bad :)

Hmm, I see! Sounds a little strange that m_player is nullptr but we need to notify observers regarding the media engine change. :-)

[Devin Rousso]

Created attachment 409764 [details]
Patch

[Eric Carlson]

Comment on attachment 409764 [details]
Patch

Thanks for adding a test!

[EWS]

Committed r267709: <https://trac.webkit.org/changeset/267709>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 409764 [details].