Bug 219810

Summary: [Cocoa] Add Experimental Vorbis support
Product: WebKit Reporter: Eric Carlson <eric.carlson>
Component: MediaAssignee: Eric Carlson <eric.carlson>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, ews-watchlist, glenn, jer.noble, philipj, sam, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Eric Carlson
Reported 2020-12-11 16:07:49 PST
Add experimental Vorbis support
Attachments
Patch (7.60 KB, patch)
2020-12-11 16:16 PST, Eric Carlson 		no flags
DetailsFormatted DiffDiff
Patch (7.59 KB, patch)
2020-12-11 16:23 PST, Eric Carlson 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2020-12-11 16:08:12 PST
<rdar://problem/72242614>
Eric Carlson
Comment 2 2020-12-11 16:16:29 PST
Created attachment 416063 [details] Patch
Jer Noble
Comment 3 2020-12-11 16:19:52 PST
Comment on attachment 416063 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416063&action=review > Source/WebCore/page/RuntimeEnabledFeatures.h:399 > + bool m_VorbisDecoderEnabled { false }; Nit: the capitalization here is weird.
Eric Carlson
Comment 4 2020-12-11 16:23:06 PST
Created attachment 416066 [details] Patch
Jer Noble
Comment 5 2020-12-11 17:18:45 PST
Comment on attachment 416066 [details] Patch Clearing flags on attachment: 416066 Committed r270722: <https://trac.webkit.org/changeset/270722>
Jer Noble
Comment 6 2020-12-11 17:18:47 PST
All reviewed patches have been landed. Closing bug.
Sam Weinig
Comment 7 2020-12-19 11:11:34 PST
Comment on attachment 416066 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416066&action=review > Source/WebCore/platform/graphics/cocoa/WebMAudioUtilitiesCocoa.mm:181 > + if (!RuntimeEnabledFeatures::sharedFeatures().vorbisDecoderEnabled()) > + return false; Hey Eric, this is a layering violation in WebCore. By rule, you cannot use the RuntimeEnabledFeatures in the platform/ part of WebCore (platform is the platform abstraction layer and therefore should not be directly influence by WebCore level concepts like this). The pattern I suggest for things like this having the platform level classes provide some configuration options (much like any API would) and having the parts of WebCore outside the platform directory that need to use platform concepts pass in setting state using those configuration options.
Eric Carlson
Comment 8 2020-12-20 22:18:02 PST
Comment on attachment 416066 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416066&action=review >> Source/WebCore/platform/graphics/cocoa/WebMAudioUtilitiesCocoa.mm:181 >> + return false; > > Hey Eric, this is a layering violation in WebCore. By rule, you cannot use the RuntimeEnabledFeatures in the platform/ part of WebCore (platform is the platform abstraction layer and therefore should not be directly influence by WebCore level concepts like this). > > The pattern I suggest for things like this having the platform level classes provide some configuration options (much like any API would) and having the parts of WebCore outside the platform directory that need to use platform concepts pass in setting state using those configuration options. Oops! https://bugs.webkit.org/show_bug.cgi?id=220052 cleans this up a bit.
Note You need to log in before you can comment on or make changes to this bug.