Bug 234753

Summary: [Cocoa] Hang in AVTrackPrivateAVFObjCImpl::bitrate()
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: MediaAssignee: Jer Noble <jer.noble>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, ap, calvaris, cdumez, changseok, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jean-yves.avenard, peng.liu6, philipj, ryuan.choi, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
eric.carlson: review+
Patch for landing
none
[fast-cq] Patch for landing none

Description Jer Noble 2021-12-30 11:23:33 PST
[Cocoa] Hang in AVTrackPrivateAVFObjCImpl::bitrate()
Comment 1 Jer Noble 2021-12-30 12:43:16 PST
Created attachment 448104 [details]
Patch
Comment 2 Jer Noble 2021-12-30 13:04:40 PST
Created attachment 448106 [details]
Patch
Comment 3 Jer Noble 2021-12-31 12:19:39 PST
Created attachment 448128 [details]
Patch
Comment 4 Jer Noble 2021-12-31 12:28:00 PST
Created attachment 448129 [details]
Patch
Comment 5 Jer Noble 2021-12-31 12:32:03 PST
Created attachment 448130 [details]
Patch
Comment 6 Jer Noble 2021-12-31 12:39:05 PST
Created attachment 448131 [details]
Patch
Comment 7 Jer Noble 2021-12-31 14:14:14 PST
Created attachment 448133 [details]
Patch
Comment 8 Jer Noble 2021-12-31 17:10:55 PST
Created attachment 448138 [details]
Patch
Comment 9 Jer Noble 2021-12-31 21:29:08 PST
Created attachment 448141 [details]
Patch
Comment 10 Alexey Proskuryakov 2022-01-02 15:49:52 PST
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?
Comment 11 Jer Noble 2022-01-03 09:18:44 PST
(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.
Comment 12 Jean-Yves Avenard [:jya] 2022-01-04 01:41:15 PST
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
Comment 13 Eric Carlson 2022-01-04 09:17:36 PST
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)
Comment 14 Peng Liu 2022-01-04 09:20:03 PST
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?
Comment 15 Jer Noble 2022-01-04 09:46:02 PST
(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!
Comment 16 Jer Noble 2022-01-04 09:51:21 PST
(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.
Comment 17 Jer Noble 2022-01-04 09:55:15 PST
Created attachment 448305 [details]
Patch for landing
Comment 18 Jer Noble 2022-01-04 10:06:23 PST
Created attachment 448306 [details]
[fast-cq] Patch for landing
Comment 19 Jer Noble 2022-01-04 10:09:46 PST
<rdar://86855896>
Comment 20 EWS 2022-01-04 10:10:05 PST
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].