Bug 214529 – [Cocoa] Add experimental MSE WebM parser

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

[Cocoa] Add experimental MSE WebM parser

[Radar WebKit Bug Importer]

<rdar://problem/65782467>

[Jer Noble]

Created attachment 404658 [details]
Patch

[Jer Noble]

Created attachment 404659 [details]
Patch

[Jer Noble]

Created attachment 404660 [details]
Patch

[Jer Noble]

Created attachment 404661 [details]
Patch

[Jer Noble]

Created attachment 404671 [details]
Patch

[Jer Noble]

Created attachment 404672 [details]
Patch

[Jer Noble]

Created attachment 404673 [details]
Patch

[Jer Noble]

Created attachment 404699 [details]
Patch

[Jer Noble]

Created attachment 404718 [details]
Patch

[Jer Noble]

Created attachment 404800 [details]
Patch

[Jer Noble]

Created attachment 404801 [details]
Patch

[Jer Noble]

Created attachment 404802 [details]
Patch

[Jer Noble]

Created attachment 404803 [details]
Patch

[Eric Carlson]

Comment on attachment 404803 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404803&action=review

> Source/ThirdParty/libwebrtc/ChangeLog:25
> +        (vp9_parser::Vp9HeaderParser::subsampling_y const):
> +        * libwebrtc.xcodeproj/project.pbxproj:
> +
> [phone]  Jer Noble  <[e-mail]>

Oops, double ChangeLog entry

> Source/WebCore/ChangeLog:183
> +        (WebCore::VideoTrackPrivateWebM::trackIndex const):
> +        * platform/graphics/cocoa/VideoTrackPrivateWebM.h: Copied from Source/WebCore/platform/graphics/cocoa/VP9UtilitiesCocoa.h.
> +
> [phone]  Jer Noble  <[e-mail]>
> +
> +        [Cocoa] Add experimental MSE WebM parser

Ditto

> Source/WebKit/ChangeLog:13
> +        * Shared/WebPreferences.yaml:
> +
> [phone]  Jer Noble  <[e-mail]>
> +
> +        [Cocoa] Add experimental MSE WebM parser

Ditto

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferParserAVFObjC.mm:59
> +    WebCore::SourceBufferParserAVFObjC* _parent;

It would be cleaner to make this a WeakPtr and not reset _parent in the SourceBufferParserAVFObjC destructor.

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferParserAVFObjC.mm:92
> +- (void)invalidate
> +{
> +    [_parser setDelegate:nil];
> +    _parser = nullptr;
> +}

It looks like this is no longer used. If it is to be used, shouldn't it invalidate _parent?

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferParserAVFObjC.mm:172
> +            CFTypeRef originalFormat = CMFormatDescriptionGetExtension(description, originalFormatKey);

If `CMFormatDescriptionGetMediaSubType` is soft-linked, shouldn't this too?

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferParserAVFObjC.mm:253
> +    callOnMainThread([this, strongThis = makeRef(*this), asset = retainPtr(asset)] {

Rather than taking a strong ref, why not make a WeakPtr and bail if it is NULL? Any processing done after the parser is only alive because of the ref won't really be used anyway.

Here and in all of the `callOnMainThread` blocks in this class.

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferParserAVFObjC.mm:285
> +            }

Although it is unlikely to be reached, it might be worth adding an `ASSERT_NOT_REACHED` as a final else block here.

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferParserAVFObjC.mm:326
> +    m_willProvideContentKeyRequestInitializationDataForTrackIDCallback(trackID);

Is this guaranteed to be non-null?

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferParserAVFObjC.mm:334
> +    m_didProvideContentKeyRequestInitializationDataForTrackIDCallback(WTFMove(initData), trackID);

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.h:95
> +    void willProvideContentKeyRequestInitializationDataForTrackID(uint64_t trackID);
> +    void didProvideContentKeyRequestInitializationDataForTrackID(Ref<Uint8Array>&&, uint64_t trackID, Box<BinarySemaphore>);

Nit: the parameter name isn't necessary.

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.h:102
> +    void trackDidChangeSelected(VideoTrackPrivate&, bool selected);
> +    void trackDidChangeEnabled(AudioTrackPrivate&, bool enabled);

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.h:149
> +    void didProvideMediaDataForTrackID(Ref<MediaSample>&&, uint64_t trackID, const String& mediaType);

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.h:169
> +    void didBecomeReadyForMoreSamples(uint64_t trackID);

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:518
> +        // We must call synchronously to the main thread, as the AVStreamSession must be associated
> +        // with the streamDataParser before the delegate method returns.

Any reason to not return immediately if weakThis is null?

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:538
> +        Box<BinarySemaphore> hasSessionSemaphore = Box<BinarySemaphore>::create();

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:559
> +        parser->appendData(WTFMove(data));

Ditto

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:705
> +    auto trackID = track.id().string().toUInt64();
>  

ASSERT(trackID)

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:731
> +    auto trackID = track.id().string().toUInt64();
>  

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:743
>          ALLOW_NEW_API_WITHOUT_GUARDS_BEGIN

Is this still necessary?

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:929
> +    auto trackID = trackIDString.string().toUInt64();

ASSERT(trackID)

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:971
> +    auto trackID = trackIDString.string().toUInt64();

Ditto

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:1083
> +    auto trackID = trackIDString.string().toUInt64();

Ditto

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:1141
> +    auto trackID = trackIDString.string().toUInt64();

Ditto

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:1166
> +    auto trackID = trackIDString.string().toUInt64();

Ditto

> Source/WebCore/platform/graphics/avfoundation/objc/VideoTrackPrivateMediaSourceAVFObjC.h:31
> +#include <wtf/Function.h>

Is this necessary?

> Source/WebCore/platform/graphics/cocoa/SourceBufferParser.cpp:51
> +RefPtr<SourceBufferParser> SourceBufferParser::create(const ContentType& contentType)
> +{
> +    if (WTF::equalIgnoringASCIICase(contentType.containerType(), "audio/webm") || WTF::equalIgnoringASCIICase(contentType.containerType(), "video/webm"))
> +        return adoptRef(new SourceBufferParserWebM());
> +    return adoptRef(new SourceBufferParserAVFObjC());
> +}

This should check isWebmParserAvailable().

> Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.cpp:175
> +        return adoptRef(*new MediaDescriptionWebM(WTFMove(track)));

ASSERT(isWebmParserAvailable());

> Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.cpp:202
> +        if (!codecID.startsWith("V_") && !codecID.startsWith("A_") && !codecID.startsWith("S_")) {

The string constants should use `_str`, e.g. `"V_"_str`

[Jer Noble]

(In reply to Eric Carlson from comment #15)
> Comment on attachment 404803 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404803&action=review
> 
> > Source/ThirdParty/libwebrtc/ChangeLog:25
> > +        (vp9_parser::Vp9HeaderParser::subsampling_y const):
> > +        * libwebrtc.xcodeproj/project.pbxproj:
> > +
> > [phone]  Jer Noble  <[e-mail]>
> 
> Oops, double ChangeLog entry

Whoops! (to this and the next two)

> > Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferParserAVFObjC.mm:59
> > +    WebCore::SourceBufferParserAVFObjC* _parent;
> 
> It would be cleaner to make this a WeakPtr and not reset _parent in the
> SourceBufferParserAVFObjC destructor.

Unfortunately this can't work. The WeakPtr will almost always be used in a different thread than the one it was created in, and that's a no-no for WeakPtrs.

> > Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferParserAVFObjC.mm:92
> > +- (void)invalidate
> > +{
> > +    [_parser setDelegate:nil];
> > +    _parser = nullptr;
> > +}
> 
> It looks like this is no longer used. If it is to be used, shouldn't it
> invalidate _parent?

_parent is reset in the parent's destructor.  But you're right that we no longer call it. I think that's because I chose to store m_parser as a Ref<> rather than a RefPtr<>, so we can't just destroy it.  I think I'll add a virtual `void invalidate()` method to the virtual base class.

> > Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferParserAVFObjC.mm:172
> > +            CFTypeRef originalFormat = CMFormatDescriptionGetExtension(description, originalFormatKey);
> 
> If `CMFormatDescriptionGetMediaSubType` is soft-linked, shouldn't this too?

We've got this in CoreMediaSoftLink.h:

```
#define CMFormatDescriptionGetExtensions softLink_CoreMedia_CMFormatDescriptionGetExtensions
```

So we don't technically need to use the softLink_* version.

> > Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferParserAVFObjC.mm:253
> > +    callOnMainThread([this, strongThis = makeRef(*this), asset = retainPtr(asset)] {
> 
> Rather than taking a strong ref, why not make a WeakPtr and bail if it is
> NULL? Any processing done after the parser is only alive because of the ref
> won't really be used anyway.

WeakPtrs don't work across threads, which is a super bummer, and I would have done it that way if it were possible.

> Here and in all of the `callOnMainThread` blocks in this class.
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferParserAVFObjC.mm:285
> > +            }
> 
> Although it is unlikely to be reached, it might be worth adding an
> `ASSERT_NOT_REACHED` as a final else block here.

Wouldn't that cause crashes if AVStreamDataParser ever started handling text or metadata tracks?  I guess we'd find out really quick when they did.  Could we do an ERROR_LOG() instead?

> > Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferParserAVFObjC.mm:326
> > +    m_willProvideContentKeyRequestInitializationDataForTrackIDCallback(trackID);
> 
> Is this guaranteed to be non-null?

Ooh, good point. I'll fix this.

> > Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.h:95
> > +    void willProvideContentKeyRequestInitializationDataForTrackID(uint64_t trackID);
> > +    void didProvideContentKeyRequestInitializationDataForTrackID(Ref<Uint8Array>&&, uint64_t trackID, Box<BinarySemaphore>);
> 
> Nit: the parameter name isn't necessary.

Eh, for this case I'd like to leave it. It's the one place that declares exactly what that parameter means.

> > Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:518
> > +        // We must call synchronously to the main thread, as the AVStreamSession must be associated
> > +        // with the streamDataParser before the delegate method returns.
> 
> Any reason to not return immediately if weakThis is null?

Threads again. You can pass a WeakPtr to another thread so long as you never access it.

> > Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:538
> > Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:705
> > +    auto trackID = track.id().string().toUInt64();
> >  
> 
> ASSERT(trackID)

I fixed it so that zero was a valid trackID, since there's nothing keeping WebM from using a zero in its track_uuid field.

```
    HashMap<uint64_t, RetainPtr<AVSampleBufferAudioRenderer>, DefaultHash<uint64_t>, WTF::UnsignedWithZeroKeyHashTraits<uint64_t>> m_audioRenderers;
```

Of course, now they can't use `-1` as a UUID, but they couldn't before either.

> > Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:743
> >          ALLOW_NEW_API_WITHOUT_GUARDS_BEGIN
> 
> Is this still necessary?

Probably for downlevel builds?

> > Source/WebCore/platform/graphics/avfoundation/objc/VideoTrackPrivateMediaSourceAVFObjC.h:31
> > +#include <wtf/Function.h>
> 
> Is this necessary?

Probably not!

> > Source/WebCore/platform/graphics/cocoa/SourceBufferParser.cpp:51
> > +RefPtr<SourceBufferParser> SourceBufferParser::create(const ContentType& contentType)
> > +{
> > +    if (WTF::equalIgnoringASCIICase(contentType.containerType(), "audio/webm") || WTF::equalIgnoringASCIICase(contentType.containerType(), "video/webm"))
> > +        return adoptRef(new SourceBufferParserWebM());
> > +    return adoptRef(new SourceBufferParserAVFObjC());
> > +}
> 
> This should check isWebmParserAvailable().

Good catch.  This should probably just re-use `SourceBufferParserWebM::isContentTypeSupported()` rather than check "video/webm" and "audio/webm" explicitly, and that code already checks isWebmParserAvailable() (as well as checking that libwebrtc is present with a weak link check).

> > Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.cpp:175
> > +        return adoptRef(*new MediaDescriptionWebM(WTFMove(track)));
> 
> ASSERT(isWebmParserAvailable());

Ok.

> > Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.cpp:202
> > +        if (!codecID.startsWith("V_") && !codecID.startsWith("A_") && !codecID.startsWith("S_")) {
> 
> The string constants should use `_str`, e.g. `"V_"_str`

String::startsWith() has explicit support for constexpr `char *` string literal params, so this shouldn't be necessary.

[Jer Noble]

Created attachment 404868 [details]
Patch for landing

[EWS]

Committed r264685: <https://trac.webkit.org/changeset/264685>

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

[Jer Noble]

Committed follow-up internal build fix: r264693 <https://trac.webkit.org/r264693>