[webkit-reviews] review granted: [Bug 214529] [Cocoa] Add experimental MSE WebM parser : [Attachment 404803] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jul 21 12:51:46 PDT 2020


Eric Carlson <eric.carlson at apple.com> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 214529: [Cocoa] Add experimental MSE WebM parser
https://bugs.webkit.org/show_bug.cgi?id=214529

Attachment 404803: Patch

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




--- Comment #15 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 404803
  --> https://bugs.webkit.org/attachment.cgi?id=404803
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:
> +
> +2020-07-19  Jer Noble  <jer.noble at apple.com>

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.
> +
> +2020-07-19  Jer Noble  <jer.noble at apple.com>
> +
> +	   [Cocoa] Add experimental MSE WebM parser

Ditto

> Source/WebKit/ChangeLog:13
> +	   * Shared/WebPreferences.yaml:
> +
> +2020-07-19  Jer Noble  <jer.noble at apple.com>
> +
> +	   [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(initD
ata), 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.m
m: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.m
m:538
> +	   Box<BinarySemaphore> hasSessionSemaphore =
Box<BinarySemaphore>::create();

Ditto.

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

Ditto

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

ASSERT(trackID)

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

Ditto.

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

Is this still necessary?

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

ASSERT(trackID)

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

Ditto

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

Ditto

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

Ditto

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

Ditto

>
Source/WebCore/platform/graphics/avfoundation/objc/VideoTrackPrivateMediaSource
AVFObjC.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`


More information about the webkit-reviews mailing list