[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