Re: Datahandle State API changes

[Tim Janik <timj gtk org>]

On Mon, 6 Nov 2006, Stefan Westerfeld wrote:

> There is one issue I was unsure with, and that is the actual meaning of
> the gsl_data_handle_get_source() function. I had assumed that this
> function would return the datahandle which provided the data, and thus,
> for the resampling case, it should return the unresampled datahandle.
>
> So I added a get_source() method to the resampling datahandle. However,
> some other datahandles where I find it obvious to implement get_source
> in a similar way, for instance the reversed handle, do not have any
> get_source function. Especially, there is no chain_handle_get_source()
> function in gsldatahandle.c, and any handle derived from the chained
> handle doesn't have a get_source function.
>
> Additionally, there is no documentation for
> gsl_data_handle_get_source(), so there is no way to look up whats the
> intended meaning. So maybe I was wrong about implementing it for the
> resampling handle, or maybe it should be implemented for more handles.

it usually is a good idea to look at the actual use cases to find out 
about an unknown function. in this case:
  bse_storage_put_data_handle()
    do    /* skip comment or cache handles */
      {
        test_handle = tmp_handle;
        tmp_handle = gsl_data_handle_get_source (test_handle);
      }
    while (tmp_handle);   /* skip comment or cache handles */ 
so:
no, resmaplers should not implement get_source, that function is only intended
for unrolling the datahandle chain as long as the data (length, number of
channels, accuracy) stays exactly the same.

>   Cu... Stefan
>
> Index: bse/gsldatahandle-vorbis.c
> ===================================================================
> --- bse/gsldatahandle-vorbis.c	(revision 4068)
> +++ bse/gsldatahandle-vorbis.c	(working copy)
> @@ -365,6 +365,7 @@ static GslDataHandleFuncs dh_vorbis_vtab
>   dh_vorbis_read,
>   dh_vorbis_close,
>   NULL,
> +  NULL,
>   dh_vorbis_destroy,
> };
>
> Index: bse/gsldatahandle-mad.c
> ===================================================================
> --- bse/gsldatahandle-mad.c	(revision 4068)
> +++ bse/gsldatahandle-mad.c	(working copy)
> @@ -673,6 +673,7 @@ static GslDataHandleFuncs dh_mad_vtable
>   dh_mad_read,
>   dh_mad_close,
>   NULL,
> +  NULL,
>   dh_mad_destroy,
> };
>
> Index: bse/ChangeLog
> ===================================================================
> --- bse/ChangeLog	(revision 4068)
> +++ bse/ChangeLog	(working copy)
> @@ -1,3 +1,19 @@
> +Mon Nov  6 13:12:50 2006  Stefan Westerfeld  <stefan space twc de>
> +
> +	* gsldatahandle.[hc]: Added new gsl_data_handle_get_state_length
> +	function for datahandles, with a corresponding vtable entry. For
> +	filtering datahandles (such as lowpass handles), which are usually
> +	stateful, it returns the filter state length.
> +
> +	* gsldatahandle-vorbis.c:
> +	* gsldatahandle-mad.c:
> +	* bsedatahandle-resample.cc:
> +	* tests/loophandle.c: Implement the get_state_length datahandle
> +	method.
> +
> +	* tests/resamplehandle.cc: Test the resampler get_state_length
> +	function.
> +
> Sun Nov  5 04:23:07 2006  Tim Janik  <timj gtk org>
>
> 	* tests/filtertest.cc (random_filter_tests): extended fixed orders to 32
> Index: bse/gsldatahandle.c
> ===================================================================
> --- bse/gsldatahandle.c	(revision 4068)
> +++ bse/gsldatahandle.c	(working copy)
> @@ -195,6 +195,41 @@ gsl_data_handle_get_source (GslDataHandl
>   return src_handle;
> }
>
> +/**
> + * @param data_handle	a DataHandle
> + * @return		the state length of the data handle
> + *
> + * Some data handles produce output samples from an input data handle,

s/some/most/

> + * like filtering and resampling datahandles. Usually, they have an internal

s/usually they/some/

> + * state which means that the value of one input sample affects not only one
> + * output sample, but some samples before and some samples after the

s/and/or/ (since that depends on the filter type)

> + * "corresponding" output sample.
> + *
> + * Often the state is symmetric, so that the number of output samples affected
> + * before and after the "corresponding" output sample is the same. Then the
> + * function returns this number. If the state is asymmetric, this function
> + * shall return the maximum of the two numbers.
> + *
> + * If multiple data handles are cascaded (for instance when resampling a

hm, "cascaded"? i'd rather say "nested" (or "chained") here.

> + * filtered signal), the function propagates the state length, so that the
> + * state of the chain of all operations together is returned.

"state of the chain"? shouldn't that be "accumulated state length" or so?

> + *
> + * Note: This function can only be used while the data handle is opened.
> + *
> + * This function is MT-safe and may be called from any thread.
> + */
> +int64
> +gsl_data_handle_get_state_length (GslDataHandle *dhandle)
> +{
> +  g_return_val_if_fail (dhandle != NULL, -1);
> +  g_return_val_if_fail (dhandle->open_count > 0, -1);
> +
> +  GSL_SPIN_LOCK (&dhandle->mutex);
> +  int64 state_length = dhandle->vtable->get_state_length ? dhandle->vtable->get_state_length (dhandle) : 0;
> +  GSL_SPIN_UNLOCK (&dhandle->mutex);
> +  return state_length;
> +}
> +
> int64
> gsl_data_handle_length (GslDataHandle *dhandle)
> {
> @@ -355,6 +390,7 @@ gsl_data_handle_new_mem (guint         n
>     mem_handle_read,
>     mem_handle_close,
>     NULL,
> +    NULL,
>     mem_handle_destroy,
>   };
>   MemHandle *mhandle;
> @@ -500,6 +536,14 @@ xinfo_get_source_handle (GslDataHandle *
>   return chandle->src_handle;
> }
>
> +static int64
> +xinfo_get_state_length (GslDataHandle *dhandle)
> +{
> +  XInfoHandle *chandle = (XInfoHandle*) dhandle;
> +  return gsl_data_handle_get_state_length (chandle->src_handle);
> +}
> +
> +
> static GslDataHandle*
> xinfo_data_handle_new (GslDataHandle *src_handle,
>                        gboolean       clear_xinfos,
> @@ -511,6 +555,7 @@ xinfo_data_handle_new (GslDataHandle *sr
>     xinfo_handle_read,
>     xinfo_handle_close,
>     xinfo_get_source_handle,
> +    xinfo_get_state_length,
>     xinfo_handle_destroy,
>   };
>   SfiRing *dest_added = NULL, *dest_remove = NULL;
> @@ -686,6 +731,12 @@ chain_handle_close (GslDataHandle *dhand
>   gsl_data_handle_close (chandle->src_handle);
> }
>
> +static int64
> +chain_handle_get_state_length (GslDataHandle *dhandle)
> +{
> +  ChainHandle *chandle = (ChainHandle*) dhandle;
> +  return gsl_data_handle_get_state_length (chandle->src_handle);
> +}
>
> /* --- reversed handle --- */
> static void
> @@ -745,6 +796,7 @@ gsl_data_handle_new_reverse (GslDataHand
>     reverse_handle_read,
>     chain_handle_close,
>     NULL,
> +    chain_handle_get_state_length,
>     reverse_handle_destroy,
>   };
>   ReversedHandle *rhandle;
> @@ -853,6 +905,7 @@ gsl_data_handle_new_translate (GslDataHa
>     cut_handle_read,
>     chain_handle_close,
>     NULL,
> +    chain_handle_get_state_length,
>     cut_handle_destroy,
>   };
>   CutHandle *chandle;
> @@ -1032,6 +1085,14 @@ insert_handle_read (GslDataHandle *dhand
>   return orig_n_values - n_values;
> }
>
> +static int64
> +insert_handle_get_state_length (GslDataHandle *dhandle)
> +{
> +  InsertHandle *ihandle = (InsertHandle*) dhandle;
> +  return gsl_data_handle_get_state_length (ihandle->src_handle);
> +}
> +
> +
> GslDataHandle*
> gsl_data_handle_new_insert (GslDataHandle *src_handle,
> 			    guint          paste_bit_depth,
> @@ -1045,6 +1106,7 @@ gsl_data_handle_new_insert (GslDataHandl
>     insert_handle_read,
>     insert_handle_close,
>     NULL,
> +    insert_handle_get_state_length,
>     insert_handle_destroy,
>   };
>   InsertHandle *ihandle;
> @@ -1161,6 +1223,7 @@ gsl_data_handle_new_looped (GslDataHandl
>     loop_handle_read,
>     chain_handle_close,
>     NULL,
> +    chain_handle_get_state_length,
>     loop_handle_destroy,
>   };
>   LoopHandle *lhandle;
> @@ -1259,6 +1322,13 @@ dcache_handle_get_source_handle (GslData
>   return chandle->dcache->dhandle;
> }
>
> +static int64
> +dcache_handle_get_state_length (GslDataHandle *dhandle)
> +{
> +  DCacheHandle *chandle = (DCacheHandle*) dhandle;
> +  return gsl_data_handle_get_state_length (chandle->dcache->dhandle);
> +}
> +
> GslDataHandle*
> gsl_data_handle_new_dcached (GslDataCache *dcache)
> {
> @@ -1267,6 +1337,7 @@ gsl_data_handle_new_dcached (GslDataCach
>     dcache_handle_read,
>     dcache_handle_close,
>     dcache_handle_get_source_handle,
> +    dcache_handle_get_state_length,
>     dcache_handle_destroy,
>   };
>   DCacheHandle *dhandle;
> @@ -1495,6 +1566,7 @@ gsl_wave_handle_new (const gchar      *f
>     wave_handle_read,
>     wave_handle_close,
>     NULL,
> +    NULL,
>     wave_handle_destroy,
>   };
>   WaveHandle *whandle;

rest looks pretty good.

> Index: bse/bsedatahandle-resample.cc
> ===================================================================
> --- bse/bsedatahandle-resample.cc	(revision 4068)
> +++ bse/bsedatahandle-resample.cc	(working copy)
> @@ -136,7 +136,7 @@ protected:
>   }
>
>   /* implemented by upsampling and downsampling datahandle */
> -  virtual BseResampler2Mode mode	() = 0;
> +  virtual BseResampler2Mode mode	() const = 0;
>   virtual int64		    read_frame  (int64 frame) = 0;
>
> public:
> @@ -222,18 +222,38 @@ public:
>
>     return n_values;
>   }
> +  GslDataHandle*
> +  get_source() const
> +  {
> +    return gsl_data_handle_get_source (m_src_handle);
> +  }
> +  int64
> +  get_state_length() const
> +  {
> +    int64 source_state_length = gsl_data_handle_get_state_length (m_src_handle);
> +    if (source_state_length < 0)
> +      return source_state_length;

huh? why should a datahandle every return a negative state?

> +    if (mode() == BSE_RESAMPLER2_MODE_UPSAMPLE)
> +      source_state_length *= 2;
> +    else
> +      source_state_length = (source_state_length + 1) / 2;
> +
> +    if (m_resamplers.size())
> +      {
> +	/* For fractional delays, a delay of 10.5 for instance means that input[0]
> +	 * affects samples 10 and 11 are affected, and thus the state length we
> +	 * assume for that case is 11.
> +	 */

the comment needs rewording, you use "affect" two times.

> +	int64 per_channel_state = ceil (m_resamplers[0]->delay());
> +	return source_state_length + per_channel_state * m_dhandle.setup.n_channels;
> +      }
> +    // should never happen
> +    return -1;

eeeek!
-1?
how is that *possibly* a valid state?

note that gsl_data_handle_get_state_length() was not
defined/discussed/documented as a function that can possibly fail,
so why don't you return 0 here?

> +  }
>   static GslDataHandle*
>   dh_create (DataHandleResample2 *cxx_dh)
>   {
> -    static GslDataHandleFuncs dh_vtable =
> -    {
> -      dh_open,
> -      dh_read,
> -      dh_close,
> -      NULL,
> -      dh_destroy,
> -    };

huh?

>     if (cxx_dh->m_init_ok)
>       {
> 	cxx_dh->m_dhandle.vtable = &dh_vtable;
> @@ -246,9 +266,10 @@ public:
> 	return NULL;
>       }
>   }
> -
> private:
> /* for the "C" API (vtable) */
> +  static GslDataHandleFuncs dh_vtable;
> +
>   static DataHandleResample2*
>   dh_cast (GslDataHandle *dhandle)
>   {
> @@ -278,6 +299,26 @@ private:
>   {
>     return dh_cast (dhandle)->read (voffset, n_values, values);
>   }
> +  static GslDataHandle*
> +  dh_get_source (GslDataHandle *dhandle)
> +  {
> +    return dh_cast (dhandle)->get_source();
> +  }
> +  static int64
> +  dh_get_state_length (GslDataHandle *dhandle)
> +  {
> +    return dh_cast (dhandle)->get_state_length();
> +  }
> +};
> +
> +GslDataHandleFuncs DataHandleResample2::dh_vtable =
> +{
> +  dh_open,
> +  dh_read,
> +  dh_close,
> +  dh_get_source,
> +  dh_get_state_length,
> +  dh_destroy,
> };

erk. declaring the table inside the class is not neccessary
and defines an additional external symbol. decls should be
moved into the innermost scope possible, so please revert that
part by moving the table back into dh_create.

> class DataHandleUpsample2 : public DataHandleResample2
> @@ -291,7 +332,7 @@ public:
>       m_dhandle.name = g_strconcat (m_src_handle->name, "// #upsample2 /", NULL);
>   }
>   BseResampler2Mode
> -  mode()
> +  mode() const
>   {
>     return BSE_RESAMPLER2_MODE_UPSAMPLE;
>   }
> @@ -366,7 +407,7 @@ public:
>   {
>   }
>   BseResampler2Mode
> -  mode()
> +  mode() const
>   {
>     return BSE_RESAMPLER2_MODE_DOWNSAMPLE;
>   }

---
ciaoTJ