Bug 236109 - [Cocoa] Adopt -streamDataParser:didProvideContentKeySpecifier:forTrackID: delegate callback
Summary: [Cocoa] Adopt -streamDataParser:didProvideContentKeySpecifier:forTrackID: del...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-02-03 16:38 PST by Jer Noble
Modified: 2022-02-28 17:00 PST (History)
22 users (show)

See Also:


Attachments
Patch (67.83 KB, patch)
2022-02-03 17:03 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (69.38 KB, patch)
2022-02-07 13:46 PST, Jer Noble
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (70.19 KB, patch)
2022-02-07 16:44 PST, Jer Noble
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (80.60 KB, patch)
2022-02-07 18:51 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (82.38 KB, patch)
2022-02-08 10:37 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (73.37 KB, patch)
2022-02-11 11:20 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (73.66 KB, patch)
2022-02-11 18:21 PST, Jer Noble
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (75.32 KB, patch)
2022-02-11 19:48 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (75.90 KB, patch)
2022-02-11 21:44 PST, Jer Noble
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (89.48 KB, patch)
2022-02-11 23:55 PST, Jer Noble
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (75.20 KB, patch)
2022-02-15 08:43 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (75.22 KB, patch)
2022-02-16 09:14 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (18.01 KB, patch)
2022-02-25 14:35 PST, Jer Noble
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (18.07 KB, patch)
2022-02-25 15:02 PST, Jer Noble
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (18.09 KB, patch)
2022-02-25 15:16 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (18.28 KB, patch)
2022-02-26 15:45 PST, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2022-02-03 16:38:31 PST
[Cocoa] Adopt -streamDataParser:didProvideContentKeySpecifier:forTrackID: delegate callback
Comment 1 Jer Noble 2022-02-03 17:03:51 PST
Created attachment 450839 [details]
Patch
Comment 2 Jer Noble 2022-02-07 13:46:34 PST
Created attachment 451147 [details]
Patch
Comment 3 Jer Noble 2022-02-07 16:44:14 PST
Created attachment 451174 [details]
Patch
Comment 4 Jer Noble 2022-02-07 18:51:46 PST
Created attachment 451191 [details]
Patch
Comment 5 Jer Noble 2022-02-08 10:37:37 PST
Created attachment 451270 [details]
Patch
Comment 6 Eric Carlson 2022-02-08 15:09:49 PST
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
> +2022-02-07  Jer Noble  <jer.noble@apple.com>
> +
> +        [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
Comment 7 Radar WebKit Bug Importer 2022-02-10 16:39:17 PST
<rdar://problem/88785844>
Comment 8 Jer Noble 2022-02-11 11:20:11 PST
Created attachment 451730 [details]
Patch for landing
Comment 9 EWS 2022-02-11 11:58:11 PST
Tools/Scripts/svn-apply failed to apply attachment 451270 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Comment 10 Jer Noble 2022-02-11 18:21:23 PST
Created attachment 451768 [details]
Patch for landing
Comment 11 Jer Noble 2022-02-11 19:48:34 PST
Created attachment 451770 [details]
Patch for landing
Comment 12 Jer Noble 2022-02-11 21:44:07 PST
Created attachment 451773 [details]
Patch for landing
Comment 13 Jer Noble 2022-02-11 23:55:05 PST
Created attachment 451774 [details]
Patch for landing
Comment 14 EWS 2022-02-12 08:58:20 PST
ChangeLog entry in Source/WebCore/ChangeLog is not at the top of the file.
Comment 15 Jer Noble 2022-02-15 08:43:52 PST
Created attachment 452027 [details]
Patch for landing
Comment 16 EWS 2022-02-15 11:29:18 PST
ChangeLog entry in Source/WTF/ChangeLog contains OOPS!.
Comment 17 Jer Noble 2022-02-16 09:14:46 PST
Created attachment 452201 [details]
Patch for landing
Comment 18 EWS 2022-02-16 12:51:14 PST
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].
Comment 19 Jer Noble 2022-02-25 14:35:08 PST
Reopening to attach new patch.
Comment 20 Jer Noble 2022-02-25 14:35:12 PST
Created attachment 453264 [details]
Patch
Comment 21 Jer Noble 2022-02-25 15:02:35 PST
Created attachment 453267 [details]
Patch
Comment 22 Jer Noble 2022-02-25 15:16:05 PST
Created attachment 453271 [details]
Patch
Comment 23 Jer Noble 2022-02-26 15:45:32 PST
Created attachment 453313 [details]
Patch
Comment 24 Eric Carlson 2022-02-28 09:20:59 PST
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?
Comment 25 Jer Noble 2022-02-28 09:24:06 PST
(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.
Comment 26 EWS 2022-02-28 17:00:42 PST
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].