Bug 230841

Summary: Add an experimental VideoTrackConfiguration class and accessor on VideoTrack
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: MediaAssignee: 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 Flags
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing
ews-feeder: commit-queue-
Patch for landing
ews-feeder: commit-queue-
Patch for landing
ews-feeder: commit-queue-
Patch for landing
ews-feeder: commit-queue-
Patch for landing
ews-feeder: commit-queue-
Patch for landing
ews-feeder: commit-queue-
Patch for landing
ews-feeder: commit-queue-
Patch for landing
ews-feeder: commit-queue-
Patch for landing
none
[fast-cq] Follow up fix
none
Follow-up patch none

Description Jer Noble 2021-09-27 09:01:44 PDT
Add an experimental VideoTrackConfiguration class and accessor on VideoTrack
Comment 1 Jer Noble 2021-09-27 09:33:34 PDT
Created attachment 439362 [details]
Patch
Comment 2 Eric Carlson 2021-09-27 10:04:09 PDT
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
Comment 3 Radar WebKit Bug Importer 2021-10-04 09:02:34 PDT
<rdar://problem/83837876>
Comment 4 Jer Noble 2021-10-22 20:05:44 PDT
Created attachment 442244 [details]
Patch
Comment 5 Jer Noble 2021-10-22 21:39:22 PDT
Created attachment 442251 [details]
Patch
Comment 6 Jer Noble 2021-10-22 22:18:33 PDT
Created attachment 442253 [details]
Patch
Comment 7 Jean-Yves Avenard [:jya] 2021-10-22 22:22:45 PDT
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?
Comment 8 Jer Noble 2021-10-23 09:13:29 PDT
Created attachment 442260 [details]
Patch
Comment 9 Jer Noble 2021-10-23 09:19:18 PDT
(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.
Comment 10 Jer Noble 2021-10-23 09:21:20 PDT
Created attachment 442263 [details]
Patch
Comment 11 Jer Noble 2021-10-23 17:25:09 PDT
Created attachment 442285 [details]
Patch
Comment 12 Jer Noble 2021-11-18 12:42:59 PST
Created attachment 444720 [details]
Patch
Comment 13 Jer Noble 2021-11-18 14:26:19 PST
Created attachment 444737 [details]
Patch
Comment 14 Jer Noble 2021-11-18 15:17:58 PST
Created attachment 444745 [details]
Patch
Comment 15 Jer Noble 2021-11-18 15:39:23 PST
Created attachment 444746 [details]
Patch
Comment 16 Eric Carlson 2021-11-18 18:07:55 PST
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
Comment 17 Jer Noble 2021-11-19 23:26:26 PST
Created attachment 444884 [details]
Patch
Comment 18 Jer Noble 2021-11-20 10:02:45 PST
Created attachment 444900 [details]
Patch
Comment 19 Jer Noble 2021-11-20 13:01:42 PST
Created attachment 444904 [details]
Patch
Comment 20 Jer Noble 2021-11-20 15:39:31 PST
(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.
Comment 21 Jer Noble 2021-11-20 15:50:24 PST
Created attachment 444908 [details]
Patch for landing
Comment 22 Jer Noble 2021-11-30 09:33:47 PST
Created attachment 445428 [details]
Patch for landing
Comment 23 Jer Noble 2021-11-30 11:52:36 PST
Created attachment 445450 [details]
Patch for landing
Comment 24 Jer Noble 2021-11-30 13:20:07 PST
Created attachment 445463 [details]
Patch for landing
Comment 25 Jer Noble 2021-12-02 14:04:43 PST
Created attachment 445771 [details]
Patch for landing
Comment 26 Jer Noble 2021-12-07 09:02:10 PST
Created attachment 446182 [details]
Patch for landing
Comment 27 Jer Noble 2021-12-07 21:36:27 PST
Created attachment 446288 [details]
Patch for landing
Comment 28 Jer Noble 2021-12-08 10:59:29 PST
Created attachment 446386 [details]
Patch for landing
Comment 29 EWS 2021-12-08 16:51:23 PST
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].
Comment 30 Robert Jenner 2021-12-09 15:56:51 PST
Reverted r286754 for reason:

Broke 2 tests on all mac an iOS

Committed r286813 (245049@trunk): <https://commits.webkit.org/245049@trunk>
Comment 31 Jer Noble 2021-12-10 15:22:38 PST
Created attachment 446827 [details]
Patch for landing
Comment 32 EWS 2021-12-11 01:02:03 PST
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].
Comment 33 Jer Noble 2021-12-13 09:51:18 PST
Reopening to attach new patch.
Comment 34 Jer Noble 2021-12-13 09:51:21 PST
Created attachment 447019 [details]
[fast-cq] Follow up fix
Comment 35 Jer Noble 2021-12-13 09:51:55 PST
Apologies, managed to re-land my patch without the test fix.
Comment 36 EWS 2021-12-13 09:55:37 PST
Committed r286953 (?): <https://commits.webkit.org/r286953>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447019 [details].
Comment 37 Jer Noble 2021-12-15 08:29:44 PST
Reopening to attach new patch.
Comment 38 Jer Noble 2021-12-15 08:29:48 PST
Created attachment 447235 [details]
Follow-up patch
Comment 39 EWS 2021-12-16 14:20:02 PST
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].
Comment 40 Yusuke Suzuki 2022-01-15 15:50:34 PST
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.
Comment 41 Yusuke Suzuki 2022-01-15 15:52:03 PST
(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
Comment 42 Yusuke Suzuki 2022-01-19 13:32:19 PST
I wonder if this patch broke 32bit build in https://bugs.webkit.org/show_bug.cgi?id=235372
Comment 43 Yusuke Suzuki 2022-01-20 12:18:48 PST
(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.