Bug 236109 – [Cocoa] Adopt -streamDataParser:didProvideContentKeySpecifier:forTrackID: delegate callback

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

[Jer Noble]

[Cocoa] Adopt -streamDataParser:didProvideContentKeySpecifier:forTrackID: delegate callback

[Jer Noble]

Created attachment 450839 [details]
Patch

[Jer Noble]

Created attachment 451147 [details]
Patch

[Jer Noble]

Created attachment 451174 [details]
Patch

[Jer Noble]

Created attachment 451191 [details]
Patch

[Jer Noble]

Created attachment 451270 [details]
Patch

[Eric Carlson]

Comment on attachment 451270 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaSampleAVFObjC.h:66
> +    PlatformSample::Type platformSampleType() const override { return PlatformSample::CMSampleBufferType; }

final

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:1251
> +        return;

Worth ASSERTing in a debug build?

> Tools/ChangeLog:15
> [phone]  Jer Noble  <[e-mail]>
> +
> +        [Cocoa] Adopt -streamDataParser:didProvideContentKeySpecifier:forTrackID: delegate callback
> +        https://bugs.webkit.org/show_bug.cgi?id=236109
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [Cocoa] Adopt -streamDataParser:didProvideContentKeySpecifier:forTrackID: delegate callback
> +        https://bugs.webkit.org/show_bug.cgi?id=236109
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Source/WebCore:
> +
> +        The current set of delegate callback methods have a significant shortcoming: clients must

This is in the wrong file

[Radar WebKit Bug Importer]

<rdar://problem/88785844>

[Jer Noble]

Created attachment 451730 [details]
Patch for landing

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 451270 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Jer Noble]

Created attachment 451768 [details]
Patch for landing

[Jer Noble]

Created attachment 451770 [details]
Patch for landing

[Jer Noble]

Created attachment 451773 [details]
Patch for landing

[Jer Noble]

Created attachment 451774 [details]
Patch for landing

[EWS]

ChangeLog entry in Source/WebCore/ChangeLog is not at the top of the file.

[Jer Noble]

Created attachment 452027 [details]
Patch for landing

[EWS]

ChangeLog entry in Source/WTF/ChangeLog contains OOPS!.

[Jer Noble]

Created attachment 452201 [details]
Patch for landing

[EWS]

Committed r289946 (247348@main): <https://commits.webkit.org/247348@main>

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

[Jer Noble]

Reopening to attach new patch.

[Jer Noble]

Created attachment 453264 [details]
Patch

[Jer Noble]

Created attachment 453267 [details]
Patch

[Jer Noble]

Created attachment 453271 [details]
Patch

[Jer Noble]

Created attachment 453313 [details]
Patch

[Eric Carlson]

Comment on attachment 453313 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferParserAVFObjC.mm:229
> +#if HAVE(AVCONTENTKEYSPECIFIER)
> +    if (MediaSessionManagerCocoa::sampleBufferContentKeySessionSupportEnabled())
> +        m_delegate = adoptNS([[WebAVStreamDataParserWithKeySpecifierListener alloc] initWithParser:m_parser.get() parent:this]);
> +    else
> +#endif

You already do runtime checks to make sure AVSampleBufferAudioRenderer and AVSampleBufferDisplayLayer conform to the new protocols, so can you get rid of the HAVE(AVCONTENTKEYSPECIFIER) here and above so you can always create a WebAVStreamDataParserWithKeySpecifierListener to make this easier to read and follow?

[Jer Noble]

(In reply to Eric Carlson from comment #24)
> Comment on attachment 453313 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453313&action=review
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferParserAVFObjC.mm:229
> > +#if HAVE(AVCONTENTKEYSPECIFIER)
> > +    if (MediaSessionManagerCocoa::sampleBufferContentKeySessionSupportEnabled())
> > +        m_delegate = adoptNS([[WebAVStreamDataParserWithKeySpecifierListener alloc] initWithParser:m_parser.get() parent:this]);
> > +    else
> > +#endif
> 
> You already do runtime checks to make sure AVSampleBufferAudioRenderer and
> AVSampleBufferDisplayLayer conform to the new protocols, so can you get rid
> of the HAVE(AVCONTENTKEYSPECIFIER) here and above so you can always create a
> WebAVStreamDataParserWithKeySpecifierListener to make this easier to read
> and follow?

Probably not, because we'd have to get rid of the HAVE() checks around the WebAVStreamDataParserWithKeySpecifierListener declaration as well, and that object references AVContentKeySpecifier, which won't be defined when HAVE(AVCONTENTKEYSPECIFIER) is false.

[EWS]

Committed r290621 (247894@main): <https://commits.webkit.org/247894@main>

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