Bug 234753 – [Cocoa] Hang in AVTrackPrivateAVFObjCImpl::bitrate()

• Component: Media
• Status: RESOLVED FIXED

[Jer Noble]

[Cocoa] Hang in AVTrackPrivateAVFObjCImpl::bitrate()

[Jer Noble]

Created attachment 448104 [details]
Patch

[Jer Noble]

Created attachment 448106 [details]
Patch

[Jer Noble]

Created attachment 448128 [details]
Patch

[Jer Noble]

Created attachment 448129 [details]
Patch

[Jer Noble]

Created attachment 448130 [details]
Patch

[Jer Noble]

Created attachment 448131 [details]
Patch

[Jer Noble]

Created attachment 448133 [details]
Patch

[Jer Noble]

Created attachment 448138 [details]
Patch

[Jer Noble]

Created attachment 448141 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 448141 [details]
Patch

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

> LayoutTests/media/track/video-track-alternate-groups-expected.txt:26
> -TRACK id:2 kind:main language:en
> -TRACK id:4 kind:main language:fr
> -TRACK id:6 kind:main language:es
> +TRACK id:3 kind:main language:eng
> +TRACK id:5 kind:main language:fra
> +TRACK id:7 kind:main language:spa

Huh, this is not just a "testExpectedEventually" change. Is this patch fixing more than originally intended?

[Jer Noble]

(In reply to Alexey Proskuryakov from comment #10)
> Comment on attachment 448141 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448141&action=review
> 
> > LayoutTests/media/track/video-track-alternate-groups-expected.txt:26
> > -TRACK id:2 kind:main language:en
> > -TRACK id:4 kind:main language:fr
> > -TRACK id:6 kind:main language:es
> > +TRACK id:3 kind:main language:eng
> > +TRACK id:5 kind:main language:fra
> > +TRACK id:7 kind:main language:spa
> 
> Huh, this is not just a "testExpectedEventually" change. Is this patch
> fixing more than originally intended?

This is a difference between how AVFoundation reports the languages from AVAssetTrack and AVMediaSelectionOption. We now use the former when present over the latter, which is more correct and now matches the language code when selection groups are not present.

So this is a correctness progression as well as a hang progression.

[Jean-Yves Avenard [:jya]]

Comment on attachment 448141 [details]
Patch

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

LGTM

> Source/WebCore/platform/graphics/PlatformAudioTrackConfiguration.h:71
> +    return true;

return decoder.decode(..) && decoder.decode(…)
?

> Source/WebCore/platform/graphics/PlatformVideoTrackConfiguration.h:71
> +        return false;

Same return decoder.decode(…) && ….

> Source/WebKit/GPUProcess/media/AudioTrackPrivateRemoteConfiguration.h:54
> +        if (!decoder.decode(configuration.enabled))

return decoder.decode() &&..

> Source/WebKit/GPUProcess/media/TrackPrivateRemoteConfiguration.h:64
> +        return true;

Same

[Eric Carlson]

Comment on attachment 448141 [details]
Patch

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

> Source/WebCore/platform/graphics/VideoTrackPrivate.h:70
> +        m_configuration = configuration;

m_configuration = WTFMove(configuration)

[Peng Liu]

Comment on attachment 448141 [details]
Patch

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

> Source/WebCore/Modules/webcodecs/VideoColorSpace.h:46
> +    void setState(const VideoColorSpaceInit& state) { m_state = state; }

Nit. Not sure "state" and "setState" are the best names.

> Source/WebCore/platform/graphics/PlatformAudioTrackConfiguration.h:45
> +    return a.codec == b.codec

Nit. Not an issue. It looks strange to use the `codec` property here, which is a property of `PlatformTrackConfiguration`.

>> Source/WebCore/platform/graphics/VideoTrackPrivate.h:70
>> +        m_configuration = configuration;
> 
> m_configuration = WTFMove(configuration)

Nit. WTFMove().

> Source/WebCore/platform/graphics/avfoundation/AVTrackPrivateAVFObjCImpl.mm:89
> +    [m_assetTrack  loadValuesAsynchronouslyForKeys:assetTrackConfigurationKeyNames() completionHandler:[weakThis = WeakPtr(this)] () mutable {

Nit. An extra space.

> Source/WebCore/platform/graphics/avfoundation/AVTrackPrivateAVFObjCImpl.mm:90
> +        RunLoop::main().dispatch([weakThis = WTFMove(weakThis)] {

Can we use `callOnMainRunLoop()` here?

[Jer Noble]

(In reply to Jean-Yves Avenard [:jya] from comment #12)
> Comment on attachment 448141 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448141&action=review
> 
> LGTM
> 
> > Source/WebCore/platform/graphics/PlatformAudioTrackConfiguration.h:71
> > +    return true;
> 
> return decoder.decode(..) && decoder.decode(…)
> ?

Good idea; much simpler!

[Jer Noble]

(In reply to Eric Carlson from comment #13)
> Comment on attachment 448141 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448141&action=review
> 
> > Source/WebCore/platform/graphics/VideoTrackPrivate.h:70
> > +        m_configuration = configuration;
> 
> m_configuration = WTFMove(configuration)

Ok.

(In reply to Peng Liu from comment #14)
> Comment on attachment 448141 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448141&action=review
> 
> > Source/WebCore/Modules/webcodecs/VideoColorSpace.h:46
> > +    void setState(const VideoColorSpaceInit& state) { m_state = state; }
> 
> Nit. Not sure "state" and "setState" are the best names.

Well, one other option is "init", but that's also not a great name. This pattern is used elsewhere, so lets brainstorm in a different patch what a better name for this pattern would be.

> > Source/WebCore/platform/graphics/PlatformAudioTrackConfiguration.h:45
> > +    return a.codec == b.codec
> 
> Nit. Not an issue. It looks strange to use the `codec` property here, which
> is a property of `PlatformTrackConfiguration`.

Yeah, I'll look into this after as well.

> >> Source/WebCore/platform/graphics/VideoTrackPrivate.h:70
> >> +        m_configuration = configuration;
> > 
> > m_configuration = WTFMove(configuration)
> 
> Nit. WTFMove().

Ok.

> > Source/WebCore/platform/graphics/avfoundation/AVTrackPrivateAVFObjCImpl.mm:89
> > +    [m_assetTrack  loadValuesAsynchronouslyForKeys:assetTrackConfigurationKeyNames() completionHandler:[weakThis = WeakPtr(this)] () mutable {
> 
> Nit. An extra space.

Ok.

> > Source/WebCore/platform/graphics/avfoundation/AVTrackPrivateAVFObjCImpl.mm:90
> > +        RunLoop::main().dispatch([weakThis = WTFMove(weakThis)] {
> 
> Can we use `callOnMainRunLoop()` here?

Sure, it's slightly shorter but exactly the same code in the end.

[Jer Noble]

Created attachment 448305 [details]
Patch for landing

[Jer Noble]

Created attachment 448306 [details]
[fast-cq] Patch for landing

[Jer Noble]

<rdar://86855896>

[EWS]

Committed r287574 (245705@main): <https://commits.webkit.org/245705@main>

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