[vlc-devel] [PATCH] wav: add RF64 support

[Tobias Rapp]

Rémi Denis-Courmont wrote:
> Le mardi 30 avril 2013 16:54:07, Tobias Rapp a écrit :
> > Add support for wave files > 4GB. See also the EBU specification at
> > http://tech.ebu.ch/docs/tech/tech3306-2009.pdf
> > 
> > ---
> >  include/vlc_codecs.h |   12 ++++++++++++
> >  modules/demux/wav.c  |   48 ++++++++++++++++++++++++++++++++++++++++++++
> > +--- 2 files changed, 57 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/vlc_codecs.h b/include/vlc_codecs.h
> > index f2b8daf..db09924 100644
> > --- a/include/vlc_codecs.h
> > +++ b/include/vlc_codecs.h
> > @@ -109,6 +109,18 @@ _WAVEHEADER {
> >  } WAVEHEADER;
> >  #endif /* _WAVEHEADER_ */
> > 
> > +#ifndef _WAVEDATASIZE64_
> > +#define _WAVEDATASIZE64_
> > +typedef struct
> > +ATTR_PACKED
> > +_WAVEDATASIZE64 {
> > +    uint64_t RiffSize;
> > +    uint64_t DataSize;
> > +    uint64_t SampleCount;
> > +    uint32_t TableLength;
> > +} WAVEDATASIZE64;
> > +#endif /* _WAVEDATASIZE64_ */
> > +
> 
> Errm, is this actually defined by Windows? If not, I don't see the point in 
> pretending it is. Also note that packed is not portable.

No, it is not. Have updated the patch to work without a packed struct, see
attached file.

> >  #ifndef _VLC_BITMAPINFOHEADER_
> >  #define _VLC_BITMAPINFOHEADER_
> > 
> > diff --git a/modules/demux/wav.c b/modules/demux/wav.c
> > index 6aefc92..de7e740 100644
> > --- a/modules/demux/wav.c
> > +++ b/modules/demux/wav.c
> > @@ -61,7 +61,7 @@ struct demux_sys_t
> >      es_out_id_t     *p_es;
> > 
> >      int64_t         i_data_pos;
> > -    unsigned int    i_data_size;
> > +    int64_t         i_data_size;
> 
> Why is this signed?

Because it is often combined with i_data_pos and compared to the result of
stream_Tell, both of them are signed int64. Have added an overflow check
similar to how it is done in stream_Tell in the updated patch.

> > 
> >      unsigned int    i_frame_size;
> >      int             i_frame_samples;
> > @@ -100,18 +100,22 @@ static int Open( vlc_object_t * p_this )
> >      demux_sys_t *p_sys;
> > 
> >      const uint8_t *p_peek;
> > +    bool           b_is_rf64;
> >      unsigned int   i_size;
> >      unsigned int   i_extended;
> >      const char    *psz_name;
> > 
> >      WAVEFORMATEXTENSIBLE *p_wf_ext = NULL;
> >      WAVEFORMATEX         *p_wf = NULL;
> > +    WAVEDATASIZE64       *p_wds = NULL;
> > 
> >      /* Is it a wav file ? */
> >      if( stream_Peek( p_demux->s, &p_peek, 12 ) < 12 )
> >          return VLC_EGENERIC;
> > 
> > -    if( memcmp( p_peek, "RIFF", 4 ) || memcmp( &p_peek[8], "WAVE", 4 ) )
> > +    b_is_rf64 = ( memcmp( p_peek, "RF64", 4 ) == 0 );
> > +    if( ( !b_is_rf64 && memcmp( p_peek, "RIFF", 4 ) ) ||
> > +	  memcmp( &p_peek[8], "WAVE", 4 ) )
> >      {
> >          return VLC_EGENERIC;
> >      }
> > @@ -123,6 +127,7 @@ static int Open( vlc_object_t * p_this )
> >          return VLC_ENOMEM;
> > 
> >      p_sys->p_es           = NULL;
> > +    p_sys->i_data_size    = 0;
> >      p_sys->i_chans_to_reorder = 0;
> >      p_sys->i_channel_mask = 0;
> > 
> > @@ -130,6 +135,41 @@ static int Open( vlc_object_t * p_this )
> >      if( stream_Read( p_demux->s, NULL, 12 ) != 12 )
> >          goto error;
> > 
> > +    if( b_is_rf64 )
> > +    {
> > +        /* search datasize64 chunk */
> > +        if( ChunkFind( p_demux, "ds64", &i_size ) )
> > +        {
> > +            msg_Err( p_demux, "cannot find 'ds64' chunk" );
> > +            goto error;
> > +        }
> > +        i_size += 2;
> 
> Integer overflow.

Fixed.

Note that a similar code pattern is used for reading the wave format chunk.
Will send a separate patch for that later.

> > +        if( i_size < sizeof( WAVEDATASIZE64 ) )
> > +        {
> > +            msg_Err( p_demux, "invalid 'ds64' chunk" );
> > +            goto error;
> > +        }
> > +        if( stream_Read( p_demux->s, NULL, 8 ) != 8 )
> > +            goto error;
> > +
> > +        /* load datasize64 */
> > +        p_wds = malloc( i_size );
> > +        if( unlikely( !p_wds ) )
> > +             goto error;
> > +
> > +        i_size -= 2;
> > +        if( stream_Read( p_demux->s, p_wds, i_size ) != (int)i_size ||
> > +            ( ( i_size & 1 ) && stream_Read( p_demux->s, NULL, 1 ) != 1 )
> > ) +        {
> > +            msg_Err( p_demux, "cannot load 'ds64' chunk" );
> > +            free( p_wds );
> > +            goto error;
> > +        }
> > +        p_sys->i_data_size = GetQWLE(&p_wds->DataSize);
> > +
> > +        free( p_wds );
> > +    }
> > +
> >      /* search fmt chunk */
> >      if( ChunkFind( p_demux, "fmt ", &i_size ) )
> >      {
> > @@ -365,11 +405,13 @@ static int Open( vlc_object_t * p_this )
> > 
> >      msg_Dbg( p_demux, "found %s audio format", psz_name );
> > 
> > -    if( ChunkFind( p_demux, "data", &p_sys->i_data_size ) )
> > +    if( ChunkFind( p_demux, "data", &i_size ) )
> >      {
> >          msg_Err( p_demux, "cannot find 'data' chunk" );
> >          goto error;
> >      }
> > +    if( !b_is_rf64 || i_size < UINT32_MAX )
> 
> Isn't this a tautology?

Not really. The RF64 spec states:

If the 32-bit value in the [chunk size] field is not "-1" (= FFFFFFFF hex)
then this 32-bit value is used. If the 32-bit value in the field is "-1" the
64-bit value in the 'ds64' chunk is used instead.

> > +        p_sys->i_data_size = i_size;
> >      if( stream_Read( p_demux->s, NULL, 8 ) != 8 )
> >          goto error;
> >      p_sys->i_data_pos = stream_Tell( p_demux->s );
> 

Thanks for the review,
Tobias
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-wav-add-RF64-support.patch
Type: text/x-patch
Size: 3393 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20130502/a410eaa6/attachment.bin>