Bug 216935 - Declare render quantum size constant in AudioUtilities.h
Summary: Declare render quantum size constant in AudioUtilities.h
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 212611
  Show dependency treegraph
 
Reported: 2020-09-24 10:22 PDT by Chris Dumez
Modified: 2020-09-24 11:33 PDT (History)
14 users (show)

See Also:


Attachments
Patch (57.80 KB, patch)
2020-09-24 10:24 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (57.92 KB, patch)
2020-09-24 10:36 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (58.11 KB, patch)
2020-09-24 10:40 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2020-09-24 10:22:12 PDT
Declare render quantum size [1] constant in AudioUtilities.h so that it can be used in both platform/ and Modules/webaudio/. Also update the code so that we have a single constant instead of many.

[1] https://www.w3.org/TR/webaudio/#render-quantum-size
Comment 1 Chris Dumez 2020-09-24 10:24:54 PDT
Created attachment 409591 [details]
Patch
Comment 2 Chris Dumez 2020-09-24 10:36:41 PDT
Created attachment 409595 [details]
Patch
Comment 3 Chris Dumez 2020-09-24 10:40:07 PDT
Created attachment 409596 [details]
Patch
Comment 4 Eric Carlson 2020-09-24 10:49:04 PDT
Comment on attachment 409596 [details]
Patch

Nice!
Comment 5 Sam Weinig 2020-09-24 11:04:06 PDT
Comment on attachment 409596 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Declare render quantum size [1] constant in AudioUtilities.h so that it can be used in both
> +        platform/ and Modules/webaudio/. Also update the code so that we have a single constant
> +        instead of many.

Conceptually, this feels wrong. The intent of platform is to abstract away differences between different platforms base libraries. Having it know about a web platform concept like a specific "render quantum size" seems like a violation of that.
Comment 6 Sam Weinig 2020-09-24 11:04:46 PDT
Comment on attachment 409596 [details]
Patch

Crap, bugzilla in-air-conflict made it automatically change the review flag.

Eric, please re-do what ever change to the flag you made.
Comment 7 Chris Dumez 2020-09-24 11:06:48 PDT
(In reply to Sam Weinig from comment #5)
> Comment on attachment 409596 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=409596&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Declare render quantum size [1] constant in AudioUtilities.h so that it can be used in both
> > +        platform/ and Modules/webaudio/. Also update the code so that we have a single constant
> > +        instead of many.
> 
> Conceptually, this feels wrong. The intent of platform is to abstract away
> differences between different platforms base libraries. Having it know about
> a web platform concept like a specific "render quantum size" seems like a
> violation of that.

The situation right now is that we have different constants in platform/ and WebAudio/ that need to be kept in sync. You view it as a WebPlatform concept but I am not sure I agree, this is WebKit's render quantum size.
Comment 8 Chris Dumez 2020-09-24 11:15:19 PDT
(In reply to Chris Dumez from comment #7)
> (In reply to Sam Weinig from comment #5)
> > Comment on attachment 409596 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=409596&action=review
> > 
> > > Source/WebCore/ChangeLog:10
> > > +        Declare render quantum size [1] constant in AudioUtilities.h so that it can be used in both
> > > +        platform/ and Modules/webaudio/. Also update the code so that we have a single constant
> > > +        instead of many.
> > 
> > Conceptually, this feels wrong. The intent of platform is to abstract away
> > differences between different platforms base libraries. Having it know about
> > a web platform concept like a specific "render quantum size" seems like a
> > violation of that.
> 
> The situation right now is that we have different constants in platform/ and
> WebAudio/ that need to be kept in sync. You view it as a WebPlatform concept
> but I am not sure I agree, this is WebKit's render quantum size.

To put it in other word, the thing you are saying is bad is not new in my patch. What my patch does it merge 5+ constants (that have to be in sync) into a single one, in a common header. It does not feel that controversial to me.
Comment 9 Sam Weinig 2020-09-24 11:16:14 PDT
(In reply to Chris Dumez from comment #7)
> (In reply to Sam Weinig from comment #5)
> > Comment on attachment 409596 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=409596&action=review
> > 
> > > Source/WebCore/ChangeLog:10
> > > +        Declare render quantum size [1] constant in AudioUtilities.h so that it can be used in both
> > > +        platform/ and Modules/webaudio/. Also update the code so that we have a single constant
> > > +        instead of many.
> > 
> > Conceptually, this feels wrong. The intent of platform is to abstract away
> > differences between different platforms base libraries. Having it know about
> > a web platform concept like a specific "render quantum size" seems like a
> > violation of that.
> 
> The situation right now is that we have different constants in platform/ and
> WebAudio/ that need to be kept in sync. You view it as a WebPlatform concept
> but I am not sure I agree, this is WebKit's render quantum size.

Ok, I could buy that.
Comment 10 Sam Weinig 2020-09-24 11:16:59 PDT
(In reply to Chris Dumez from comment #8)
> (In reply to Chris Dumez from comment #7)
> > (In reply to Sam Weinig from comment #5)
> > > Comment on attachment 409596 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=409596&action=review
> > > 
> > > > Source/WebCore/ChangeLog:10
> > > > +        Declare render quantum size [1] constant in AudioUtilities.h so that it can be used in both
> > > > +        platform/ and Modules/webaudio/. Also update the code so that we have a single constant
> > > > +        instead of many.
> > > 
> > > Conceptually, this feels wrong. The intent of platform is to abstract away
> > > differences between different platforms base libraries. Having it know about
> > > a web platform concept like a specific "render quantum size" seems like a
> > > violation of that.
> > 
> > The situation right now is that we have different constants in platform/ and
> > WebAudio/ that need to be kept in sync. You view it as a WebPlatform concept
> > but I am not sure I agree, this is WebKit's render quantum size.
> 
> To put it in other word, the thing you are saying is bad is not new in my
> patch. What my patch does it merge 5+ constants (that have to be in sync)
> into a single one, in a common header. It does not feel that controversial
> to me.

Fair. 

Also, I really didn't mean to change the review flag at all. It was just this dumb site :(. Please put it back to the way it was.
Comment 11 Chris Dumez 2020-09-24 11:32:59 PDT
Committed r267541: <https://trac.webkit.org/changeset/267541>
Comment 12 Radar WebKit Bug Importer 2020-09-24 11:33:19 PDT
<rdar://problem/69515634>