Bug 216935 – Declare render quantum size constant in AudioUtilities.h

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Chris Dumez]

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

[Chris Dumez]

Created attachment 409591 [details]
Patch

[Chris Dumez]

Created attachment 409595 [details]
Patch

[Chris Dumez]

Created attachment 409596 [details]
Patch

[Eric Carlson]

Comment on attachment 409596 [details]
Patch

Nice!

[Sam Weinig]

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.

[Sam Weinig]

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.

[Chris Dumez]

(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.

[Chris Dumez]

(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.

[Sam Weinig]

(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.

[Sam Weinig]

(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.

[Chris Dumez]

Committed r267541: <https://trac.webkit.org/changeset/267541>

[Radar WebKit Bug Importer]

<rdar://problem/69515634>