[Webkit-unassigned] [Bug 34912] audio engine: add ReverbConvolver class

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=34912





--- Comment #12 from Chris Rogers <crogers at google.com>  2010-03-17 13:35:07 PST ---
Hi Jeremy, I addressed a bunch of your comments.  Once again, I really
appreciate your help.  Here are some comments on the remaining issues:

> Is this file mac specific?

No, it's a cross-platform abstraction which declares the necessary DSP
functions
It's in another patch:
https://bugs.webkit.org/show_bug.cgi?id=34452

>This should be private and you should have a ::create() factory (static
>function) that's public.

This is in regards to ReverbAccumulationBuffer which is a helper class only
used by ReverbConvolver.  It's used directly as a composed member object there,
so the object lifetime is taken care of automatically.  Dimitri Glazkov told me
it's definitely OK to have helper classes which can be used as stack-based
objects and directly as composed members of other classes with no need for
::create() and private constructor.

> Still not super happy about this solution, but I guess we should wait until it
> actually needs per-platform tuning to add in such code.

Yeah, I understand your feeling.  Unfortunately, this is the messy world of
real-time systems where scheduler performance of different OS kernel's may come
into play.  In this particular case, I think it will be OK.  But it's one of
several things to be tested and tuned (if necessary) per-platform.

> This should be an OwnPtr.

The pointer is being put either into m_backgroundStages or m_stages which are
both Vector<OwnPtr<ReverbConvolverStage> >
I *did* make the local pointer into a PassOwnPtr<ReverbConvolverStage>
I found that the compiler was not happy if I used OwnPtr in the local variable
in addition to the Vector...

> There's probably something in wtf that'll do this for you now.  Please take a
> look and consider fixing this now.

I hunted all over for a sleep(), usleep(), nanosleep() abstraction in wtf, but
did not find it.  I think a good place to put one would be in <wtf/Threading.h>
But I'm not sure how to implement that for all platforms yet.

> Here and in all of your classes, the constructors should be private and you
> should have factory functions instead.  The factory functions should return
> either PassOwnPtr<> or PassRefPtr<>.  This minimizes the number of places > where memory leaks can start and makes it clear whether it's ref counted or 
> not.

Dimitri assured me that it's OK to not use ::create() methods /
private-constructors (in order to be able to use as stack-based objects,
composed member variables, etc.).  The case where it is required is when it
will be ref-counted (RefPtr<>) which is not the case here.

> Doesn't seem like this needs to be virtual.  Only make it virtual if it's
> subclassed and needs it.

I *did* remove the virtual, but I tend to *always* use virtual for destructors
unless I know for a fact that I need the object to occupy an exact number of
bytes (to overlay a struct, for example) or if there are other clear
performance reasons not to.  The reason is that it's very easy to create
difficult to track down bugs if you later sub-class and forget the virtual.

> Be careful with this stuff.  I checked and there are other cases of
> Vector<OwnPtr<>>'s, but it seems a tad dangerous (in terms of accidentally
> claiming ownership and thus making the OwnPtr in the vector point to 0.  But,
> like I said, it's done elsewhere and you can certainly do it safely if you're
> just a little careful.

Yeah I'm trying to be pretty paranoid when using this kind of stuff.  But, I'm
not sure what the alternative is here if you want to use OwnPtr, since it is
indeed a Vector of them that I'm keeping track of...

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.