Re: [Gnash-commit] gnash ChangeLog backend/Makefile.am backend/gna...

[Bastiaan Jacques]

On Monday 24 July 2006 15:30, Tomas Groth wrote:
> Log message:
>       Added the new Gstreamer based soundbackend. It still needs some
> work!

Good stuff! Some minor nits are picked below.

> Index: backend/gnash.cpp
> ===================================================================
> RCS file: /sources/gnash/gnash/backend/gnash.cpp,v
> retrieving revision 1.46
> retrieving revision 1.47
> diff -u -b -r1.46 -r1.47
> --- backend/gnash.cpp 10 Jul 2006 13:48:06 -0000      1.46
> +++ backend/gnash.cpp 24 Jul 2006 13:30:50 -0000      1.47
> @@ -429,6 +429,10 @@
>              sound = gnash::create_sound_handler_sdl();
>              gnash::set_sound_handler(sound);
>  #endif
> +#ifdef HAVE_GST_GST_H
> +            sound = gnash::create_sound_handler_gst();
> +            gnash::set_sound_handler(sound);
> +#endif
>          }
>      }

I think we should `cvs delete' backend/gnash.cpp.

> Index: plugin/player.cpp
> ===================================================================
> RCS file: /sources/gnash/gnash/plugin/player.cpp,v
> retrieving revision 1.14
> retrieving revision 1.15
> diff -u -b -r1.14 -r1.15
> --- plugin/player.cpp 8 May 2006 21:12:24 -0000       1.14
> +++ plugin/player.cpp 24 Jul 2006 13:30:50 -0000      1.15
> @@ -340,6 +340,12 @@
>       gnash::set_sound_handler(sound);
>      }
>  #endif
> +#ifdef HAVE_GST_GST_H
> +    if (do_sound) {
> +     sound = gnash::create_sound_handler_sdl();
> +     gnash::set_sound_handler(sound);
> +    }
> +#endif
>      inst->lockDisplay();
>      render = gnash::create_render_handler_ogl();
>      gnash::set_render_handler(render);

And I think we're not using this file either anymore.


> Index: server/sound.cpp
> ===================================================================
> RCS file: /sources/gnash/gnash/server/sound.cpp,v
> retrieving revision 1.9
> retrieving revision 1.10
> diff -u -b -r1.9 -r1.10
> --- server/sound.cpp  7 Jun 2006 03:03:21 -0000       1.9
> +++ server/sound.cpp  24 Jul 2006 13:30:51 -0000      1.10
> @@ -112,7 +112,50 @@
>               }
>       };

[snip]

> +// Load a SoundStreamHead(2) tag.
> +void
> +sound_stream_head_loader(stream* in, tag_type tag, movie_definition*
> m) +{
> +#ifdef HAVE_GST_GST_H
> +     assert(tag == 18 || tag == 45);
> +
> +     // FIXME:
> +     // no character id for soundstreams... so we make one up...
> +     // This only works if there is only one stream in the movie...
> +     // The right way to do it is to make seperate structures for
> streams +     // in movie_def_impl.
> +     uint16_t        character_id = 10000;
> +
> +     // extract garbage data
> +     int     garbage = in->read_uint(8);
> +
> +     sound_handler::format_type      format = (sound_handler::format_type)
> in->read_uint(4); +   int     sample_rate = in->read_uint(2); // multiples
> of 5512.5 +   bool    sample_16bit = in->read_uint(1) ? true : false;
> +     bool    stereo = in->read_uint(1) ? true : false;
> +
> +     // checks if this is a new streams header or just one in the row
> +     if (format == 0 && sample_rate == 0 && sample_16bit == 0 && stereo
> == 0) return; +

Suggest:

if (format == 0 && sample_rate == 0 && !sample_16bit && !stereo)

> +     int     sample_count = in->read_u32();
> +     if (format == 2) garbage = in->read_uint(16);
> +
> +     static int      s_sample_rate_table[] = { 5512, 11025, 22050, 44100 };
> +
> +     log_parse("sound stream head: ch=%d, format=%d, rate=%d, 16=%d,
> stereo=%d, ct=%d\n", +                  character_id, int(format), 
> sample_rate,
> int(sample_16bit), int(stereo), sample_count); +
> +     // If we have a sound_handler, ask it to init this sound.
> +     if (s_sound_handler)
> +     {
> +             int     data_bytes = 0;
> +             unsigned char*  data = NULL;

Suggest:

if (! (sample_rate >= 0 && sample_rate <= 3)
  return;

> +
> +             int     handler_id = s_sound_handler->create_sound(
> +                     data,
> +                     data_bytes,
> +                     sample_count,
> +                     format,
> +                     s_sample_rate_table[sample_rate],
> +                     stereo,
> +                     true);
> +             sound_sample*   sam = new sound_sample_impl(handler_id);
> +             m->add_sound_sample(character_id, sam);
> +
> +             sound_sample_impl*      sam_impl = (sound_sample_impl*)
> m->get_sound_sample(10000);

I think we should be using C++-style casts wherever possible.

> +             start_stream_sound_tag* ssst = new 
> start_stream_sound_tag(); +           ssst->read(m, sam_impl);
> +
> +             delete [] data;

The current implementation of create_sound() doesn't appear to touch 
`data' at all, so deleting it isn't necessary. If you intend to do 
something with `data' in the future, you should of course leave it. But 
otherwise, you should consider just passing NULL to create_sound().

> +// Load a SoundStreamBlock tag.
> +void
> +sound_stream_block_loader(stream* in, tag_type tag,
> movie_definition* m) +{
> +#ifdef HAVE_GST_GST_H
> +     assert(tag == 19);
> +
> +     // extract garbage data
> +     int     garbage = in->read_uint(32);
> +
> +     // If we have a sound_handler, store the data with the appropiate
> sound. +      if (s_sound_handler)
> +     {
> +             int     data_bytes = 0;
> +             unsigned char*  data = NULL;
> +
> +             // @@ This is pretty awful -- lots of copying, slow reading.
> +             data_bytes = in->get_tag_end_position() - in->get_position();

Make sure that data_bytes > 0 ?

> +             data = new unsigned char[data_bytes];
> +             for (int i = 0; i < data_bytes; i++)
> +             {
> +                     data[i] = in->read_u8();
> +             }
> +
> +             // Swap bytes on behalf of the host, to make it easier for the
> handler. +            // @@ I'm assuming this is a good idea?  Most sound
> handlers will prefer native endianness?
> +             s_sound_handler->fill_stream_data(data, data_bytes);
> +
> +             delete [] data;
> +     }
> +#endif
> +}

> Index: backend/sound_handler_gst.cpp
> ===================================================================
> RCS file: backend/sound_handler_gst.cpp
> diff -N backend/sound_handler_gst.cpp
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ backend/sound_handler_gst.cpp     24 Jul 2006 13:30:50 -0000      1.1
> @@ -0,0 +1,634 @@

[snip]

> +// Use gstreamer to handle sounds.
> +struct GST_sound_handler : gnash::sound_handler
> +{
> +     // gstreamer pipeline objects
> +
> +     // the main bin containing the adder and output (sink)
> +     GstElement *pipeline;
> +
> +     GstElement *adder;
> +     GstElement *audiosink;
> +
> +     // Sound data.
> +     std::vector<sound_data*>        m_sound_data;
> +
> +     // Keeps track of numbers of playing sounds
> +     int soundsPlaying;
> +
> +     // Is the loop running?
> +     bool looping;
> +
> +     // latest sound stream we've created - we expect some data to
> arrive +      int currentStream;
> +
> +     GST_sound_handler()

: soundsPlaying(0),
  looping(false)

> +     {
> +             // init gstreamer
> +             gst_init(NULL, NULL);
> +
> +             // create main pipeline
> +             pipeline = gst_pipeline_new (NULL);
> +
> +             // create adder
> +             adder = gst_element_factory_make ("adder", NULL);
> +
> +             // create an audio sink - use oss, alsa or...? make a 
> commandline
> option? +             // we first try alsa, then oss, then esd, then...?
> +             audiosink = gst_element_factory_make ("alsasink", NULL);
> +             if (!audiosink) audiosink = gst_element_factory_make ("osssink",
> NULL); +              if (!audiosink) audiosink = gst_element_factory_make
> ("esdsink", NULL); +
> +             // Check if the creation of the gstreamer pipeline, adder and
> audiosink was a succes +              if (!pipeline || !adder || !audiosink) {
> +                     gnash::log_error("One gstreamer element could not be 
> created\n");
> +             }
> +
> +             // link adder and output to bin
> +             gst_bin_add (GST_BIN (pipeline), adder);
> +             gst_bin_add (GST_BIN (pipeline), audiosink);
> +
> +             // link adder and audiosink
> +             gst_element_link (adder, audiosink);
> +
> +             soundsPlaying = 0;
> +
> +             looping = false;
> +
> +     }
> +
> +     ~GST_sound_handler()
> +     {
> +
> +             for (int i = 0; i < m_sound_data.size(); i++) {
> +                     stop_sound(i);
> +                     delete_sound(i);
> +             }
> +             m_sound_data.clear();

You don't have to clear() m_sound_data; the vector will be destroyed 
when it goes out of scope anyway. However, it _is_ your responsibility 
to `delete' the elements of m_sound_data; clear() doesn't do that for 
you. So this would be a good place to iterate m_sound_data (preferably 
using iterators ;)) and delete the sound_data pointers.

> +
> +             gst_object_unref (GST_OBJECT (pipeline));
> +
> +     }
> +
> +
> +     virtual int     create_sound(
> +             void* data,
> +             int data_bytes,
> +             int sample_count,
> +             format_type format,
> +             int sample_rate,
> +             bool stereo,
> +             bool stream)
> +     // Called to create a sample.  We'll return a sample ID that
> +     // can be use for playing it.
> +     {
> +             // Add something similar... check gst elements?
> +             /*if (m_opened == false)
> +             {
> +                     return 0;
> +             }*/

Style nit: can you please #ifdef 0 unused code out?

> +
> +             int16_t* adjusted_data = 0;
> +             int     adjusted_size = 0;
> +
> +             sound_data *sounddata = new sound_data;
> +             if (sounddata == NULL) {

Style nit: if (!sounddata) ?


> +                     gnash::log_error("could not allocate memory for 
> sounddata !\n");
> +                     return -1;
> +             }
> +
> +             sounddata->format = format;
> +             sounddata->data_size = data_bytes;
> +             sounddata->stereo = stereo;
> +             sounddata->sample_count = sample_count;
> +             sounddata->sample_rate = sample_rate;
> +             sounddata->volume = 100;
> +
> +             switch (format)
> +             {
> +             // TODO: Do we need to do the raw-data-convert? Can't Gstreamer
> handle it? +          case FORMAT_RAW:
> +/*   caps info:
> +      audio/x-raw-int
> +                   rate: [ 1, 2147483647 ]
> +               channels: [ 1, 8 ]
> +             endianness: { 1234, 4321 }
> +                  width: 8
> +                  depth: [ 1, 8 ]
> +                 signed: { true, false }*/
> +                     /*convert_raw_data(&adjusted_data, &adjusted_size, data,
> sample_count, 1, sample_rate, stereo); +                      sounddata->data 
> =
> (guint8*) malloc(adjusted_size);
> +                     memcpy(sounddata->data, adjusted_data, adjusted_size);*/
> +
> +                     sounddata->data = (guint8*) malloc(data_bytes);

malloc() can fail and would then return NULL; you should check for that. 
Also, why aren't use using operator new?

> +                     memcpy(sounddata->data, data, data_bytes);
> +                     break;
> +
> +             case FORMAT_NATIVE16:
> +/*   caps info:
> +      audio/x-raw-int
> +                   rate: [ 1, 2147483647 ]
> +               channels: [ 1, 8 ]
> +             endianness: { 1234, 4321 }
> +                  width: 16
> +                  depth: [ 1, 16 ]
> +                 signed: { true, false }*/
> +                     /*convert_raw_data(&adjusted_data, &adjusted_size, data,
> sample_count, 2, sample_rate, stereo); +                      sounddata->data 
> =
> (guint8*) malloc(adjusted_size);
> +                     memcpy(sounddata->data, adjusted_data, adjusted_size);*/
> +
> +                     sounddata->data = (guint8*) malloc(data_bytes);
> +                     memcpy(sounddata->data, data, data_bytes);
> +                     break;
> +
> +             case FORMAT_MP3:
> +             //case FORMAT_VORBIS:
> +                     sounddata->data = (guint8*) malloc(data_bytes);
> +                     memcpy(sounddata->data, data, data_bytes);
> +
> +                     break;
> +             default:
> +                     // Unhandled format.
> +                     gnash::log_error("unknown format sound requested; this 
> demo does
> not handle it\n"); +                  break;
> +             }
> +
> +             m_sound_data.push_back(sounddata);
> +
> +
> +             if (stream) currentStream = m_sound_data.size()-1;
> +
> +             return m_sound_data.size()-1;
> +     }

Bastiaan