Re: [Audacity-devel] SampleBuffer proposal

[Joshua H. <jo...@re...>]

On Sun, 2003-05-25 at 21:12, Dominic Mazzoni wrote:
> Joshua Haberman wrote:
> > Note: I think this is post-1.2 material.
> > 
> > The samplePtr mechanism for supporting multiple sample formats has
> > always made me a little nervous with all the casting it required.  I was
> > thinking about Tino's bug report and how mistakes like that are so easy
> > to make.  It reminded me of an idea I've had for a while, which is to
> > replace the samplePtr typedef with a SampleBuffer class:
> 
> I could be wrong, but my guess is that the bug has to do with how we
> handle 24-bit files in libsndfile.  Probably either reading or writing
> them is not correct.
> 
> > class SampleBuffer
> > {
> >     // Use in place of NewSamples()
> >     SampleBuffer(int samples, sampleFormat format, channels=1);
> > 
> >     // Set the buffer's contents equal to this array of samples,
> >     // performing conversion if necessary.
> >     void SetData(float *samples, int len, int stride=1);
> >     void SetData(int *samples, int len, int stride=1);
> >     void SetData(short int *samples, int len, int stride=1);
> > 
> >     // Use in place of CopySamples
> >     SampleBuffer Copy(sampleFormat dstFormat,
> >                       int offset=0, int len=-1, // -1 for the whole buffer
> >                       int channel=-1,           // -1 for all channels
> >                       bool highQuality=true);
> > 
> >     void Clear(int channel=0, int offset=0, int len=-1);
> > 
> >     sampleFormat GetFormat();
> >     int GetNumChannels();
> >     int GetLength();
> > 
> >     // These throw an assertion if the buffer is not in the requested
> >     // format -- if you want conversion you must Copy().  Though this
> >     // adds an extra step, it also makes clear the extra memory and
> >     // CPU time conversion takes
> >     float *GetFloatData();
> >     int   *Get24Data();
> >     short int *Get16Data();
> > };
> > 
> > I'm sure that actually implementing this scheme would lead to
> > refinements in this API, but I think it's a good idea.  Advantages:
> > 
> > - type-safe; no more casting!  It's impossible to accidentally
> >   mess up sample formats (unless you cast, which you shouldn't)
> > - Nicer to use, it knows about channels and offsets and the
> >   difference between byte offsets and sample offsets.
> > 
> > This seems right to me, what does everyone else think?
> 
> I'm in favor of a type-safe way to do sample conversions.
> 
> My concerns with your proposal are:
> 
> * Verbosity - it looks like it will sometimes take more steps to
>    accomplish the same thing, or at least more characters.  If at
>    all possible I'd like to make sample conversion as concise as
>    possible so that it doesn't get in the way of the readability
>    of an existing algorithm.

I completely agree, readability is one of my primary motivations for
writing a class like this.  If this particular proposal would make for
hard-to-read code, then it should be refined to be better.  But I think
the general idea is sound, and I think it is possible to come up with a
similar system that makes for much clearer code than we have now.

> * The GetFloatData, Get16Data methods aren't typesafe enough.
>    Having an assertion failure if you call the wrong one doesn't
>    actually make it any easier to write clean code, just to debug
>    it later.  Ideally it shouldn't be possible to write bad code
>    if you use the class correctly.  Consider the wxString class,
>    for example - it's virtually foolproof.

You're right, I wasn't perfectly happy with this technique for just the
reasons you mention.

> Rather than a single class, maybe we need a parent class and three
> subclasses: FloatBuffer, Int24Buffer, and Int16Buffer.  That way
> each one only has one constructor, and one Get() method, but each
> one also has a copy constructor that lets you convert from one to
> the other very easily.

This is nice, I like it.

> One other question I have is who should own the actual array of
> samples?  Currently it looks like your class owns the samples.
> This doesn't handle the case where what you really want is to
> copy from one already allocated block of memory, to another already
> allocated block of memory.  It shouldn't be necessary to construct
> 2 or more classes and allocate/copy/deallocate just to do the
> conversion that you could currently do with one CopySamples call.

There's no reason why we couldn't write a constructor that takes
ownership of an existing chunk of memory.

> One thing I'd be willing to consider is if we could start passing
> around SampleBuffers everywhere, treating them like arrays of
> samples.  In other words, they should have an inline [] operator,
> that does bounds-checking when in debug mode.  That would be a nice
> added benefit.  I'll bet a lot of effects could be rewritten to
> use SampleBuffers exclusively instead of arrays.  It would be really
> neat to have high-level operations like FFT, max, min, etc. all work
> directly on SampleBuffers, too.

It was definitely my intention that SampleBuffers be passed everywhere. 
However I was assuming that using a [] operator would be too much
overhead for tight loops, which is why I included the GetData()
methods.  But now that I think about it I guess an inline [] operator
should be efficient enough, as long as we can be sure it gets inlined. 
This wouldn't allow a SampleBuffer to be aware of multiple channels
(since [] can only take one argument) but now that I think about it
internal buffer in Audacity are almost never more than one channel.  It
is the importers and exporters where this matters.

Josh