Summary: | [Cocoa] Hang in AVTrackPrivateAVFObjCImpl::bitrate() | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jer Noble <jer.noble> | ||||||||||||||||||||||||
Component: | Media | Assignee: | 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
Jer Noble
2021-12-30 11:23:33 PST
Created attachment 448104 [details]
Patch
Created attachment 448106 [details]
Patch
Created attachment 448128 [details]
Patch
Created attachment 448129 [details]
Patch
Created attachment 448130 [details]
Patch
Created attachment 448131 [details]
Patch
Created attachment 448133 [details]
Patch
Created attachment 448138 [details]
Patch
Created attachment 448141 [details]
Patch
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? (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 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 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 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? (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! (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. Created attachment 448305 [details]
Patch for landing
Created attachment 448306 [details]
[fast-cq] Patch for landing
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]. |