Bug 149888 – [iOS] <video> elements without audio tracks should not interrupt music

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

[iOS] <video> elements without audio tracks should not interrupt music

[Jer Noble]

Created attachment 262724 [details]
Patch

[Darin Adler]

Comment on attachment 262724 [details]
Patch

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

Not sure exactly why the Mac build is failing.

> Source/WebCore/html/HTMLMediaElement.cpp:6450
> +bool HTMLMediaElement::producesAudio() const
> +{
> +    if (m_player && m_readyState >= HAVE_METADATA)
> +        return hasAudio();
> +    return hasTagName(HTMLNames::audioTag);
> +}

This function is hard to use correctly. It can return false for a video element and then change its mind and start returning true for the same element as the element loads content. I think it can even return true for an audio element and then start returning false for the same element as the element loads content in some strange cases. What notifies interested parties that this may have changed and it’s time to check it again?

It also seems weak design for this to call hasTagName(HTMLNames::audioTag) when we already have separate classes for the audio and video elements.

Maybe override this for video element and audio element instead of media element.

Maybe a name like “producing audio” would make it a little more clear that this is a state that can change over time.

> Tools/TestWebKitAPI/Tests/WebKit/ios/AudioSessionCategoryIOS.mm:34
> +#import <UIKit/SoftLinking.h>

On the bot: TestWebKitAPI/Tests/WebKit/ios/AudioSessionCategoryIOS.mm:34:9: fatal error: 'UIKit/SoftLinking.h' file not found

[Jer Noble]

(In reply to comment #2)
> Comment on attachment 262724 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=262724&action=review
> 
> Not sure exactly why the Mac build is failing.
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:6450
> > +bool HTMLMediaElement::producesAudio() const
> > +{
> > +    if (m_player && m_readyState >= HAVE_METADATA)
> > +        return hasAudio();
> > +    return hasTagName(HTMLNames::audioTag);
> > +}
> 
> This function is hard to use correctly. It can return false for a video
> element and then change its mind and start returning true for the same
> element as the element loads content. I think it can even return true for an
> audio element and then start returning false for the same element as the
> element loads content in some strange cases. What notifies interested
> parties that this may have changed and it’s time to check it again?

Such is the nature of video on the internet; especially with HLS (and also with MSE) video can go from having an audio track to not having one without user intervention.  However, we handle this case by re-running PlatformMediaSessionManager::updateSessionState() whenever the hasAudio() or hasVideo() change in HTMLMediaElement::mediaPlayerCharacteristicChanged().

> It also seems weak design for this to call hasTagName(HTMLNames::audioTag)
> when we already have separate classes for the audio and video elements.

True.

> Maybe override this for video element and audio element instead of media
> element.
> 
> Maybe a name like “producing audio” would make it a little more clear that
> this is a state that can change over time.

Sure.

> > Tools/TestWebKitAPI/Tests/WebKit/ios/AudioSessionCategoryIOS.mm:34
> > +#import <UIKit/SoftLinking.h>
> 
> On the bot: TestWebKitAPI/Tests/WebKit/ios/AudioSessionCategoryIOS.mm:34:9:
> fatal error: 'UIKit/SoftLinking.h' file not found

I'll try to use one of the WebKit versions of that header.

[Jer Noble]

Created attachment 262801 [details]
Patch

[Darin Adler]

Comment on attachment 262801 [details]
Patch

Patch does not build on Mac.

[Jer Noble]

Looks like CanHandleRequest.cpp somehow got re-added to the TestWebKitAPI project (bad merge?).  Will fix.

[Jer Noble]

Created attachment 264438 [details]
Patch

[Eric Carlson]

Comment on attachment 264438 [details]
Patch

r=me

[Jer Noble]

Comment on attachment 264438 [details]
Patch

Whoops, forgot tests in this patch.

[Jer Noble]

Created attachment 264593 [details]
Patch for landing

[Jer Noble]

Created attachment 264595 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 264595 [details]
Patch for landing

Rejecting attachment 264595 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 264595, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/382217

[Jer Noble]

Created attachment 264794 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 264794 [details]
Patch for landing

Clearing flags on attachment: 264794

Committed r192027: <http://trac.webkit.org/changeset/192027>

[Jer Noble]

<rdar://problem/22235751>