Bug 230841 – Add an experimental VideoTrackConfiguration class and accessor on VideoTrack

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Jer Noble]

Add an experimental VideoTrackConfiguration class and accessor on VideoTrack

[Jer Noble]

Created attachment 439362 [details]
Patch

[Eric Carlson]

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

[Radar WebKit Bug Importer]

<rdar://problem/83837876>

[Jer Noble]

Created attachment 442244 [details]
Patch

[Jer Noble]

Created attachment 442251 [details]
Patch

[Jer Noble]

Created attachment 442253 [details]
Patch

[Jean-Yves Avenard [:jya]]

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?

[Jer Noble]

Created attachment 442260 [details]
Patch

[Jer Noble]

(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.

[Jer Noble]

Created attachment 442263 [details]
Patch

[Jer Noble]

Created attachment 442285 [details]
Patch

[Jer Noble]

Created attachment 444720 [details]
Patch

[Jer Noble]

Created attachment 444737 [details]
Patch

[Jer Noble]

Created attachment 444745 [details]
Patch

[Jer Noble]

Created attachment 444746 [details]
Patch

[Eric Carlson]

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

[Jer Noble]

Created attachment 444884 [details]
Patch

[Jer Noble]

Created attachment 444900 [details]
Patch

[Jer Noble]

Created attachment 444904 [details]
Patch

[Jer Noble]

(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.

[Jer Noble]

Created attachment 444908 [details]
Patch for landing

[Jer Noble]

Created attachment 445428 [details]
Patch for landing

[Jer Noble]

Created attachment 445450 [details]
Patch for landing

[Jer Noble]

Created attachment 445463 [details]
Patch for landing

[Jer Noble]

Created attachment 445771 [details]
Patch for landing

[Jer Noble]

Created attachment 446182 [details]
Patch for landing

[Jer Noble]

Created attachment 446288 [details]
Patch for landing

[Jer Noble]

Created attachment 446386 [details]
Patch for landing

[EWS]

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].

[Robert Jenner]

Reverted r286754 for reason:

Broke 2 tests on all mac an iOS

Committed r286813 (245049@trunk): <https://commits.webkit.org/245049@trunk>

[Jer Noble]

Created attachment 446827 [details]
Patch for landing

[EWS]

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].

[Jer Noble]

Reopening to attach new patch.

[Jer Noble]

Created attachment 447019 [details]
[fast-cq] Follow up fix

[Jer Noble]

Apologies, managed to re-land my patch without the test fix.

[EWS]

Committed r286953 (?): <https://commits.webkit.org/r286953>

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

[Jer Noble]

Reopening to attach new patch.

[Jer Noble]

Created attachment 447235 [details]
Follow-up patch

[EWS]

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].

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

(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

[Yusuke Suzuki]

I wonder if this patch broke 32bit build in https://bugs.webkit.org/show_bug.cgi?id=235372

[Yusuke Suzuki]

(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.