Summary: | Add an experimental VideoTrackConfiguration class and accessor on VideoTrack | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jer Noble <jer.noble> | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Component: | Media | Assignee: | Jer Noble <jer.noble> | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | annulen, benjamin, calvaris, cdumez, changseok, cmarcelo, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, hta, jean-yves.avenard, jenner, kondapallykalyan, peng.liu6, philipj, ryuan.choi, sergio, tommyw, webkit-bug-importer, ysuzuki | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
Bug Blocks: | 235275, 235372 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Jer Noble
2021-09-27 09:01:44 PDT
Created attachment 439362 [details]
Patch
Comment on attachment 439362 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=439362&action=review > Source/WebCore/Modules/webcodecs/VideoColorSpace.h:46 > + void setPrimaries(std::optional<VideoColorPrimaries>&& primaries) { m_init.primaries = primaries; } = WTFMove(... > Source/WebCore/Modules/webcodecs/VideoColorSpace.h:49 > + void setTransfer(std::optional<VideoTransferCharacteristics>&& transfer) { m_init.transfer = transfer; } Ditto > Source/WebCore/Modules/webcodecs/VideoColorSpace.h:55 > + void setfFullRange(std::optional<bool>&& fullRange) { m_init.fullRange = fullRange; } Ditto > Source/WebCore/Modules/webcodecs/VideoColorSpace.h:60 > + : m_init(init) Ditto > Source/WebCore/platform/graphics/HEVCUtilities.cpp:81 > + StringBuilder builder; > + builder.append("codecs=\"avc1."); > + builder.append(hex(parameters.profileIDC, 2)); > + builder.append(hex(parameters.constraintsFlags, 2)); > + builder.append(hex(parameters.levelIDC, 2)); > + builder.append('"'); > + return builder.toString(); Would `makeString("codecs=\"avc1.", hex(parameters.profileIDC, 2), ...)` be more efficient (I can never remember when to use one vs the other)? > Source/WebCore/platform/graphics/HEVCUtilities.cpp:205 > + StringBuilder builder; > + builder.append("codecs=\"hvc1."); > + if (parameters.generalProfileSpace) > + builder.append('A' + parameters.generalProfileSpace - 1); > + builder.append(parameters.generalProfileIDC); > + builder.append('.'); > + builder.append(OSSwapInt32(parameters.generalProfileCompatibilityFlags)); > + builder.append('.'); > + builder.append(parameters.generalTierFlag ? 'H' : 'L'); > + builder.append(parameters.generalLevelIDC); > + builder.append('"'); Ditto > Source/WebCore/platform/graphics/avfoundation/AVTrackPrivateAVFObjCImpl.mm:245 > > + Nit: extra blank line Created attachment 442244 [details]
Patch
Created attachment 442251 [details]
Patch
Created attachment 442253 [details]
Patch
Comment on attachment 442251 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=442251&action=review > Source/WebCore/platform/graphics/VideoTrackPrivate.h:66 > + virtual void setCodec(String codec) { m_codec = codec; } Should do WTFMove(codec) so that the compiler can optimise accordingly see https://clang.llvm.org/extra/clang-tidy/checks/modernize-pass-by-value.html > Source/WebCore/platform/graphics/avfoundation/MediaSelectionGroupAVFObjC.h:57 > + AVAssetTrack *assetTrack() const; Seeing that the code declare it as AVAssetTrack* shouldn't this style be enforced everywhere? Created attachment 442260 [details]
Patch
(In reply to Jean-Yves Avenard [:jya] from comment #7) > Comment on attachment 442251 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=442251&action=review > > > Source/WebCore/platform/graphics/VideoTrackPrivate.h:66 > > + virtual void setCodec(String codec) { m_codec = codec; } > > Should do WTFMove(codec) so that the compiler can optimise accordingly see > https://clang.llvm.org/extra/clang-tidy/checks/modernize-pass-by-value.html Ok. > > Source/WebCore/platform/graphics/avfoundation/MediaSelectionGroupAVFObjC.h:57 > > + AVAssetTrack *assetTrack() const; > > Seeing that the code declare it as AVAssetTrack* shouldn't this style be > enforced everywhere? `Type *name` _is_ the style for Objective-C objects: <https://webkit.org/code-style-guidelines/#pointers-non-cpp>. It's not perfectly enforced however. Created attachment 442263 [details]
Patch
Created attachment 442285 [details]
Patch
Created attachment 444720 [details]
Patch
Created attachment 444737 [details]
Patch
Created attachment 444745 [details]
Patch
Created attachment 444746 [details]
Patch
Comment on attachment 444746 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=444746&action=review > Source/WebCore/Modules/webcodecs/VideoColorSpace.h:64 > + VideoColorSpaceInit m_init { }; Nit: 'm_init' is a strange name since it is mutated by the setters. Maybe 'm_state'? > Source/WebCore/html/track/AudioTrackConfiguration.h:66 > + AudioTrackConfigurationInit m_init; Ditto > Source/WebCore/platform/graphics/VideoTrackPrivate.h:75 > + virtual void setColorSpace(PlatformVideoColorSpace colorSpace) { m_colorSpace = colorSpace; } setColorSpace(PlatformVideoColorSpace&& colorSpace) { m_colorSpace = WTFMove(colorSpace); } > Source/WebCore/platform/graphics/avfoundation/AVTrackPrivateAVFObjCImpl.mm:260 > + if (!assetTrack || !assetTrack.formatDescriptions.count) > + return nullptr; Why not assert if there is no assetTrack like in the other methods? > Source/WebCore/platform/graphics/avfoundation/AVTrackPrivateAVFObjCImpl.mm:299 > + auto assetTrack = assetTrackFor(*this); > + if (!assetTrack) { > + ASSERT_NOT_REACHED(); > + return 0; > + } > + return assetTrack.nominalFrameRate; Nit: This style should match the other methods: if (auto assetTrack = assetTrackFor(*this)) return assetTrack.nominalFrameRate; ASSERT_NOT_REACHED(); return 0; > Source/WebKit/GPUProcess/media/RemoteVideoTrackProxy.cpp:70 > + .colorSpace = m_trackPrivate->colorSpace(), can you WTFMove() this? > Source/WebKit/GPUProcess/media/TrackPrivateRemoteConfiguration.h:178 > + *codec, WTFMove > Source/WebKit/GPUProcess/media/TrackPrivateRemoteConfiguration.h:181 > + *colorSpace, Ditto Created attachment 444884 [details]
Patch
Created attachment 444900 [details]
Patch
Created attachment 444904 [details]
Patch
(In reply to Eric Carlson from comment #16) > Comment on attachment 444746 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=444746&action=review > > > Source/WebCore/Modules/webcodecs/VideoColorSpace.h:64 > > + VideoColorSpaceInit m_init { }; > > Nit: 'm_init' is a strange name since it is mutated by the setters. Maybe > 'm_state'? Ok. > > Source/WebCore/html/track/AudioTrackConfiguration.h:66 > > + AudioTrackConfigurationInit m_init; > > Ditto Ok. > > Source/WebCore/platform/graphics/VideoTrackPrivate.h:75 > > + virtual void setColorSpace(PlatformVideoColorSpace colorSpace) { m_colorSpace = colorSpace; } > > setColorSpace(PlatformVideoColorSpace&& colorSpace) { m_colorSpace = > WTFMove(colorSpace); } Ok. > > Source/WebCore/platform/graphics/avfoundation/AVTrackPrivateAVFObjCImpl.mm:260 > > + if (!assetTrack || !assetTrack.formatDescriptions.count) > > + return nullptr; > > Why not assert if there is no assetTrack like in the other methods? Because this one commonly gets hit for HLS content. I've only seen this get hit for audio tracks, though. > > Source/WebCore/platform/graphics/avfoundation/AVTrackPrivateAVFObjCImpl.mm:299 > > + auto assetTrack = assetTrackFor(*this); > > + if (!assetTrack) { > > + ASSERT_NOT_REACHED(); > > + return 0; > > + } > > + return assetTrack.nominalFrameRate; > > Nit: This style should match the other methods: > > if (auto assetTrack = assetTrackFor(*this)) > return assetTrack.nominalFrameRate; > ASSERT_NOT_REACHED(); > return 0; Ok. > > Source/WebKit/GPUProcess/media/RemoteVideoTrackProxy.cpp:70 > > + .colorSpace = m_trackPrivate->colorSpace(), > > can you WTFMove() this? Don't need to. Return values are already r-value references. > > Source/WebKit/GPUProcess/media/TrackPrivateRemoteConfiguration.h:178 > > + *codec, > > WTFMove Ok. > > Source/WebKit/GPUProcess/media/TrackPrivateRemoteConfiguration.h:181 > > + *colorSpace, > > Ditto Ok. Created attachment 444908 [details]
Patch for landing
Created attachment 445428 [details]
Patch for landing
Created attachment 445450 [details]
Patch for landing
Created attachment 445463 [details]
Patch for landing
Created attachment 445771 [details]
Patch for landing
Created attachment 446182 [details]
Patch for landing
Created attachment 446288 [details]
Patch for landing
Created attachment 446386 [details]
Patch for landing
Committed r286754 (244997@main): <https://commits.webkit.org/244997@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 446386 [details]. Reverted r286754 for reason: Broke 2 tests on all mac an iOS Committed r286813 (245049@trunk): <https://commits.webkit.org/245049@trunk> Created attachment 446827 [details]
Patch for landing
Committed r286908 (245134@main): <https://commits.webkit.org/245134@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 446827 [details]. Reopening to attach new patch. Created attachment 447019 [details]
[fast-cq] Follow up fix
Apologies, managed to re-land my patch without the test fix. Committed r286953 (?): <https://commits.webkit.org/r286953> All reviewed patches have been landed. Closing bug and clearing flags on attachment 447019 [details]. Reopening to attach new patch. Created attachment 447235 [details]
Follow-up patch
Committed r287158 (245335@main): <https://commits.webkit.org/245335@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 447235 [details]. Please do not use pas_utils.h. It is not utility header we can use (outside of bmalloc). libpas is self-contained, and it is used by the other projects. Unless it is marked as PAS_API, there is no guarantee that this function continues to exist. If we need some of these functions, define it in WTF and use it. (In reply to Yusuke Suzuki from comment #40) > Please do not use pas_utils.h. It is not utility header we can use (outside > of bmalloc). > libpas is self-contained, and it is used by the other projects. Unless it is > marked as PAS_API, there is no guarantee that this function continues to > exist. > If we need some of these functions, define it in WTF and use it. Tracked in https://bugs.webkit.org/show_bug.cgi?id=235275 I wonder if this patch broke 32bit build in https://bugs.webkit.org/show_bug.cgi?id=235372 (In reply to Yusuke Suzuki from comment #42) > I wonder if this patch broke 32bit build in > https://bugs.webkit.org/show_bug.cgi?id=235372 Yes, including pas_utils.h broke 32bit build. |