[Gnash-commit] /srv/bzr/gnash/trunk r9965: Change sound_handler::create_sound interface to take a SimpleBuffer by auto_ptr, avoiding memory copies for event sounds (and improving readability); have sound_data constructor make sure that if a SimpleBuffer is passed it's padded a MediaHandler requires; have DEFINESOUND tag loader query MediaHandler for padding bytes when allocating the buffer for sound data. Fixes an invalid read of ffmpeg on EmbeddedSoundTest.swf

[Sandro Santilli]

------------------------------------------------------------
revno: 9965
committer: Sandro Santilli <address@hidden>
branch nick: trunk
timestamp: Wed 2008-10-08 16:13:08 +0200
message:
  Change sound_handler::create_sound interface to take a SimpleBuffer by 
auto_ptr, avoiding memory copies for event sounds (and improving readability); 
have sound_data constructor make sure that if a SimpleBuffer is passed it's 
padded a MediaHandler requires; have DEFINESOUND tag loader query MediaHandler 
for padding bytes when allocating the buffer for sound data. Fixes an invalid 
read of ffmpeg on EmbeddedSoundTest.swf
modified:
  libcore/swf/tag_loaders.cpp
  libmedia/NullSoundHandler.h
  libmedia/ffmpeg/AudioDecoderFfmpeg.cpp
  libmedia/ffmpeg/sound_handler_sdl.cpp
  libmedia/ffmpeg/sound_handler_sdl.h
  libmedia/sound_handler.h

=== modified file 'libcore/swf/tag_loaders.cpp'
--- a/libcore/swf/tag_loaders.cpp       2008-09-04 15:55:54 +0000
+++ b/libcore/swf/tag_loaders.cpp       2008-10-08 14:13:08 +0000
@@ -58,6 +58,7 @@
 #include "abc_block.h"
 #include "SoundInfo.h"
 #include "gnash.h" // FileType enum
+#include "MediaHandler.h"
 
 #ifdef HAVE_ZLIB_H
 #include <zlib.h>
@@ -1241,12 +1242,18 @@
         // First it is the amount of data from file,
         // then the amount allocated at *data (it may grow)
         const unsigned dataLength = in.get_tag_end_position() - in.tell();
-        unsigned char *data = new unsigned char[dataLength];
+
+        // Allocate MediaHandler::getInputPadding() bytes more for the 
SimpleBuffer 
+        size_t allocSize = dataLength;
+        media::MediaHandler* mh = media::MediaHandler::get(); // TODO: don't 
use this static !
+        if ( mh ) allocSize += mh->getInputPaddingSize();
+
+        std::auto_ptr<SimpleBuffer> data( new SimpleBuffer(allocSize) );
 
         // dataLength is already calculated from the end of the tag, which
-        // should be inside the end of the file. TODO: check that this is tha 
case.
-        const unsigned int bytesRead = in.read(reinterpret_cast<char*>(data), 
dataLength);
-
+        // should be inside the end of the file. TODO: check that this is the 
case.
+        const unsigned int bytesRead = 
in.read(reinterpret_cast<char*>(data->data()), dataLength);
+        data->resize(bytesRead); // in case it's shorter...
         if (bytesRead < dataLength)
         {
             throw ParserException(_("Tag boundary reported past end of 
SWFStream!"));
@@ -1258,8 +1265,7 @@
 
         // Stores the sounddata in the soundhandler, and the ID returned
         // can be used to starting, stopping and deleting that sound
-        // NOTE: ownership of 'data' is transferred to the sound hanlder 
-        int    handler_id = handler->create_sound(data, dataLength, sinfo);
+        int    handler_id = handler->create_sound(data, sinfo);
 
         if (handler_id >= 0)
         {
@@ -1405,7 +1411,7 @@
 
     // Stores the sounddata in the soundhandler, and the ID returned
     // can be used to starting, stopping and deleting that sound
-    int handler_id = handler->create_sound(NULL, 0, sinfo);
+    int handler_id = handler->create_sound(std::auto_ptr<SimpleBuffer>(0), 
sinfo);
 
     m.set_loading_sound_stream_id(handler_id);
 }

=== modified file 'libmedia/NullSoundHandler.h'
--- a/libmedia/NullSoundHandler.h       2008-06-17 11:31:42 +0000
+++ b/libmedia/NullSoundHandler.h       2008-10-08 14:13:08 +0000
@@ -43,8 +43,7 @@
 
        // See dox in sound_handler.h 
        virtual int     create_sound(
-               void*           /*data*/,
-               unsigned int    /*data_bytes*/,
+               std::auto_ptr<SimpleBuffer> /*data*/,
                std::auto_ptr<SoundInfo> /*sinfo*/
                )
        {

=== modified file 'libmedia/ffmpeg/AudioDecoderFfmpeg.cpp'
--- a/libmedia/ffmpeg/AudioDecoderFfmpeg.cpp    2008-09-23 17:14:12 +0000
+++ b/libmedia/ffmpeg/AudioDecoderFfmpeg.cpp    2008-10-08 14:13:08 +0000
@@ -24,7 +24,7 @@
 #include <cmath> // for std::ceil
 #include <algorithm> // for std::copy, std::max
 
-//#define GNASH_DEBUG_AUDIO_DECODING
+#define GNASH_DEBUG_AUDIO_DECODING
 
 #ifdef FFMPEG_AUDIO2
 # define AVCODEC_DECODE_AUDIO avcodec_decode_audio2
@@ -222,7 +222,7 @@
 boost::uint8_t*
 AudioDecoderFfmpeg::decode(boost::uint8_t* input, boost::uint32_t inputSize, 
boost::uint32_t& outputSize, boost::uint32_t& decodedBytes, bool parse)
 {
-       //GNASH_REPORT_FUNCTION;
+       GNASH_REPORT_FUNCTION;
 
        if ( ! parse )
        {
@@ -344,7 +344,7 @@
 boost::uint8_t*
 AudioDecoderFfmpeg::decodeFrame(boost::uint8_t* input, boost::uint32_t 
inputSize, boost::uint32_t& outputSize)
 {
-       //GNASH_REPORT_FUNCTION;
+       GNASH_REPORT_FUNCTION;
 
         //static const unsigned int bufsize = (AVCODEC_MAX_AUDIO_FRAME_SIZE * 
3) / 2;
         static const unsigned int bufsize = AVCODEC_MAX_AUDIO_FRAME_SIZE;

=== modified file 'libmedia/ffmpeg/sound_handler_sdl.cpp'
--- a/libmedia/ffmpeg/sound_handler_sdl.cpp     2008-10-08 08:25:38 +0000
+++ b/libmedia/ffmpeg/sound_handler_sdl.cpp     2008-10-08 14:13:08 +0000
@@ -30,6 +30,9 @@
 #include "AudioDecoderSimple.h"
 #include "AudioDecoderNellymoser.h"
 
+#include "MediaHandler.h"
+
+// TODO: drop FFMPEG/GST specific stuff, use MediaHandler instead !
 #ifdef USE_FFMPEG
 #include "AudioDecoderFfmpeg.h"
 #elif defined(USE_GST)
@@ -165,42 +168,22 @@
 
 
 int    SDL_sound_handler::create_sound(
-       void* data,
-       unsigned int data_bytes,
+       std::auto_ptr<SimpleBuffer> data,
        std::auto_ptr<SoundInfo> sinfo)
-// Called to create a sample.  We'll return a sample ID that
-// can be use for playing it.
 {
 
+    log_debug("create_sound: sound format %d", sinfo->getFormat());
+
        assert(sinfo.get());
-       std::auto_ptr<sound_data> sounddata ( new sound_data );
-
-       //sounddata->data_size = data_bytes;
-       sounddata->volume = 100;
-       sounddata->soundinfo = sinfo;
-
+
+       std::auto_ptr<sound_data> sounddata ( new sound_data(data, sinfo) );
+
+    // Make sure we're the only thread accessing m_sound_data here
        boost::mutex::scoped_lock lock(_mutex);
 
-       switch (sounddata->soundinfo->getFormat())
-       {
-       case AUDIO_CODEC_MP3:
-               sounddata->append(reinterpret_cast<boost::uint8_t*>(data), 
data_bytes);
-               break;
-
-       case AUDIO_CODEC_RAW:
-       case AUDIO_CODEC_ADPCM:
-       case AUDIO_CODEC_UNCOMPRESSED:
-       case AUDIO_CODEC_NELLYMOSER:
-               sounddata->append(reinterpret_cast<boost::uint8_t*>(data), 
data_bytes);
-               break;
-
-       default:
-               // Unhandled format.
-               log_error(_("unknown sound format %d requested; gnash does not 
handle it"), (int)sounddata->soundinfo->getFormat());
-               return -1; // Unhandled format, set to NULL.
-       }
-
-       m_sound_data.push_back(sounddata.release()); // the vector takes 
ownership
+    // the vector takes ownership
+       m_sound_data.push_back(sounddata.release());
+
        int sound_id = m_sound_data.size()-1;
 
        return sound_id;
@@ -858,6 +841,42 @@
 }
 
 void
+sound_data::append(boost::uint8_t* data, unsigned int size)
+{
+    // Make sure we're always appropriately padded...
+    media::MediaHandler* mh = media::MediaHandler::get(); // TODO: don't use 
this static !
+    const size_t paddingBytes = mh ? mh->getInputPaddingSize() : 0;
+    _buf->reserve(_buf->size()+size+paddingBytes);
+       _buf->append(data, size);
+
+    // since ownership was transferred...
+       delete [] data;
+}
+
+sound_data::sound_data(std::auto_ptr<SimpleBuffer> data, 
std::auto_ptr<SoundInfo> info, int nVolume)
+    :
+    _buf(data),
+    soundinfo(info),
+    volume(nVolume)
+{
+    if ( _buf.get() )
+    {
+        // Make sure we're appropriately padded (this is an event sound)
+        media::MediaHandler* mh = media::MediaHandler::get(); // TODO: don't 
use this static !
+        const size_t paddingBytes = mh ? mh->getInputPaddingSize() : 0;
+        if ( _buf->capacity() - _buf->size() < paddingBytes ) {
+            log_error("sound_data creator didn't appropriately pad sound data. 
"
+                "We'll do now, but will cost memory copies.");
+            _buf->reserve(_buf->size()+paddingBytes);
+        }
+    }
+    else
+    {
+        _buf.reset(new SimpleBuffer());
+    }
+}
+
+void
 sound_data::clearActiveSounds()
 {
        for (ActiveSounds::iterator i=m_active_sounds.begin(), 
e=m_active_sounds.end(); i!=e; ++i)

=== modified file 'libmedia/ffmpeg/sound_handler_sdl.h'
--- a/libmedia/ffmpeg/sound_handler_sdl.h       2008-10-08 08:25:38 +0000
+++ b/libmedia/ffmpeg/sound_handler_sdl.h       2008-10-08 14:13:08 +0000
@@ -50,12 +50,22 @@
 class sound_data
 {
        /// The undecoded data
-       SimpleBuffer _buf;
+       std::auto_ptr<SimpleBuffer> _buf;
+
+    void ensureBufferPadding();
 
 public:
 
-       sound_data()
-       {}
+    /// Construct a sound with given data, info and volume.
+    //
+    /// @param data The encoded sound data. May be the NULL pointer for 
streaming sounds,
+    ///     in which case data will be appended later using ::append()
+    ///
+    /// @param info encoding info
+    ///
+    /// @pararm nVolume initial volume (0..100). Optional, defaults to 100.
+    ///
+       sound_data(std::auto_ptr<SimpleBuffer> data, std::auto_ptr<SoundInfo> 
info, int nVolume=100);
 
        ~sound_data();
 
@@ -72,26 +82,22 @@
        /// @param size
        ///     Size of the 'data' buffer.
        ///
-       void append(boost::uint8_t* data, unsigned int size)
-       {
-               _buf.append(data, size);
-               delete [] data; // since ownership was transferred...
-       }
+       void append(boost::uint8_t* data, unsigned int size);
 
        /// Return size of the data buffer
        size_t size() const 
        {
-               return _buf.size();
+               return _buf->size();
        }
 
        /// Return a pointer to the underlying buffer
        const boost::uint8_t* data() const {
-               return _buf.data();
+               return _buf->data();
        }
 
        /// Return a pointer to the underlying buffer
        boost::uint8_t* data() {
-               return _buf.data();
+               return _buf->data();
        }
 
        /// Return a pointer to an offset in the underlying buffer
@@ -100,8 +106,8 @@
        ///     An assertion will fail if pos > size()
        ///
        const boost::uint8_t* data(size_t pos) const {
-               assert(pos < _buf.size());
-               return _buf.data()+pos;
+               assert(pos < _buf->size());
+               return _buf->data()+pos;
        }
 
        /// Return a pointer to an offset in the underlying buffer
@@ -110,8 +116,8 @@
        ///     An assertion will fail if pos > size()
        ///
        boost::uint8_t* data(size_t pos) {
-               assert(pos < _buf.size());
-               return _buf.data()+pos;
+               assert(pos < _buf->size());
+               return _buf->data()+pos;
        }
 
        /// Volume for AS-sounds, range: 0-100.
@@ -342,8 +348,8 @@
        SDL_sound_handler(const std::string& wave_file);
        ~SDL_sound_handler();
 
-       /// Called to create a sound.
-       virtual int     create_sound(void* data, unsigned int data_bytes, 
std::auto_ptr<SoundInfo> sinfo);
+       // see dox in sound_handler.h
+       virtual int     create_sound(std::auto_ptr<SimpleBuffer> data, 
std::auto_ptr<SoundInfo> sinfo);
 
        /// this gets called when a stream gets more data
        virtual long    fill_stream_data(unsigned char* data, unsigned int 
data_bytes,

=== modified file 'libmedia/sound_handler.h'
--- a/libmedia/sound_handler.h  2008-10-02 07:39:45 +0000
+++ b/libmedia/sound_handler.h  2008-10-08 14:13:08 +0000
@@ -30,6 +30,7 @@
 
 #include "dsodefs.h" // for DSOEXPORT
 #include "SoundInfo.h"
+#include "SimpleBuffer.h"
 
 #include <vector>
 #include <memory>
@@ -87,28 +88,22 @@
 
        // If stereo is true, samples are interleaved w/ left sample first.
        
-       /// Create a sound buffer slot, for playing on-demand.
+       /// Create a sound buffer slot, for on-demand playback.
        //
        /// @param data
        ///     The data to be stored. For soundstream this is NULL.
-       ///     If not NULL, ownership of the data is transferred.
-       ///     The data is assumed to have been allocated using new[].
-       ///     The data is in encoded format, with format specified
-       ///     with the sinfo parameter, this is to allow on-demand
-       ///     decoding (if the sound is never played, it's never decoded).
-       ///
-       /// @param data_bytes
-       ///     The size of the data to be stored. For soundstream this is 0.
+       ///         The data is in encoded format, with format specified
+       ///         with the sinfo parameter, this is to allow on-demand
+       ///         decoding (if the sound is never played, it's never decoded).
        ///
        /// @param sinfo
        ///     A SoundInfo object contained in an auto_ptr, which contains 
info about samplerate,
        ///     samplecount, stereo and more. The SoundObject must be not-NULL!
        ///
        /// @return the id given by the soundhandler for later identification.
-       ///
+    ///
        virtual int     create_sound(
-               void*           data,
-               unsigned int    data_bytes,
+               std::auto_ptr<SimpleBuffer> data,
                std::auto_ptr<SoundInfo> sinfo
                ) = 0;