Bug 216299 – [Cocoa] PERF: Don't instantiate AVPlayer-based audio decoders or renderers if an element is initially muted.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

[Cocoa] Don't instantiate AVPlayer-based audio decoders or renderers if an element is initially muted.

[Jer Noble]

Created attachment 408296 [details]
Patch

[Peng Liu]

Comment on attachment 408296 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        audio decoding and rendering until the first time the AVPlayer is unmuted. ThisThe  means the first un-mute may

s/ThisThe/This/

> Source/WebCore/PAL/pal/spi/cocoa/AVFoundationSPI.h:77
> +@property (nonatomic, getter=_suppressesAudioRendering, setter=_setSuppressesAudioRendering:) BOOL suppressesAudioRendering

Miss a ";"?

[Jer Noble]

Created attachment 408366 [details]
Patch

[Eric Carlson]

Comment on attachment 408366 [details]
Patch

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

r=me once the bots are happy

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:946
> +        m_avPlayer.get().suppressesAudioRendering = YES;

We should always know if an element is muted or not with the above changes, and setMuted() will always be called if the muted state changes, so how about something like `m_avPlayer.get().suppressesAudioRendering = m_muted`?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1334
> +    if (!m_muted)
> +        m_avPlayer.get().suppressesAudioRendering = NO;

setMuted(true) may be called before playback starts, so something like `m_avPlayer.get().suppressesAudioRendering = m_muted` might be better.

[Eric Carlson]

Comment on attachment 408366 [details]
Patch

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

> Source/WebCore/PAL/pal/spi/cocoa/AVFoundationSPI.h:77
> +@property (nonatomic, getter=_suppressesAudioRendering, setter=_setSuppressesAudioRendering:) BOOL suppressesAudioRendering

Looks like you are missing a semi-colon

[Jer Noble]

Comment on attachment 408366 [details]
Patch

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

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:946
>> +        m_avPlayer.get().suppressesAudioRendering = YES;
> 
> We should always know if an element is muted or not with the above changes, and setMuted() will always be called if the muted state changes, so how about something like `m_avPlayer.get().suppressesAudioRendering = m_muted`?

If you look deeper in the context for this change, this is already inside a `if (m_muted)` block. And three lines above m_muted is set to `false`, so we literally have no choice but to set this unconditionally to YES on this line.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1334
>> +        m_avPlayer.get().suppressesAudioRendering = NO;
> 
> setMuted(true) may be called before playback starts, so something like `m_avPlayer.get().suppressesAudioRendering = m_muted` might be better.

That would cause suppression to kick in if the video was muted during playback.  The existing logic definitely has edge cases where suppression won't kick in (I.e., if you do `video.src = foo; video.addEventListener('loadedmetadata', event => { video.muted = true; });`), but those edge cases feel less common, and we can always expand this behavior.

[Jer Noble]

Created attachment 408375 [details]
Patch for Landing

[EWS]

Committed r266844: <https://trac.webkit.org/changeset/266844>

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

[Radar WebKit Bug Importer]

<rdar://problem/68654077>

[Chris Dumez]

I suspect this has caused <rdar://problem/68743466>.