[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Gnash-commit] gnash ChangeLog backend/Makefile.am backend/gna...
From: |
Bastiaan Jacques |
Subject: |
Re: [Gnash-commit] gnash ChangeLog backend/Makefile.am backend/gna... |
Date: |
Mon, 24 Jul 2006 16:45:04 +0200 |
User-agent: |
KMail/1.9.3 |
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