[vlc-devel] [PATCH 02/11] demux: wav: refactor chunk skip

[Thomas Guillem]

Here is what I propose:

static int ChunkSkip( demux_t *p_demux, uint32_t i_size )
{
    i_size += i_size & 1;

    if( i_size >= 65536 )
        return vlc_stream_Seek( p_demux->s,
                                vlc_stream_Tell( p_demux->s ) + i_size );

    ssize_t i_ret = vlc_stream_Read( p_demux->s, NULL, i_size );
    return i_ret < 0 || (size_t) i_ret != i_size ? VLC_EGENERIC : VLC_SUCCESS;
}


On Fri, Mar 13, 2020, at 16:47, Thomas Guillem wrote:
> I though there was an imptom in the core that triggered a seek in case 
> of big NULL read, but it's not the case.
> 
> So, indeed, Seek is better here.
> 
> On Fri, Mar 13, 2020, at 16:30, Rémi Denis-Courmont wrote:
> > Le perjantaina 13. maaliskuuta 2020, 15.54.11 EET Thomas Guillem a écrit :
> > > On Thu, Mar 12, 2020, at 17:34, Remi Denis-Courmont wrote:
> > > > Le 2020-03-12 16:05, Thomas Guillem a écrit :
> > > > > ---
> > > > > 
> > > > >  modules/demux/wav.c | 19 +++++++++++++------
> > > > >  1 file changed, 13 insertions(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/modules/demux/wav.c b/modules/demux/wav.c
> > > > > index c68f5e916eb..4f483df10c7 100644
> > > > > --- a/modules/demux/wav.c
> > > > > +++ b/modules/demux/wav.c
> > > > > @@ -116,6 +116,17 @@ static int Control( demux_t *p_demux, int
> > > > > i_query, va_list args )
> > > > > 
> > > > >                                     i_query, args );
> > > > >  
> > > > >  }
> > > > > 
> > > > > +static int ChunkSkip( demux_t *p_demux, size_t i_size )
> > > > > +{
> > > > > +    if( vlc_stream_Read( p_demux->s, NULL, i_size ) != (ssize_t)i_size
> > > > > )
> > > > > +        return VLC_EGENERIC;
> > > > 
> > > > Can you really assume that i_size <= SSIZE_MAX here even on 32-bits
> > > > platforms?
> > > > 
> > > > Also i_size += i_size & 1; would simplify.
> > > 
> > > Thanks for your comment.
> > > 
> > > Is something like that OK ?
> > > 
> > > static int ChunkSkip( demux_t *p_demux, uint32_t i_size )
> > > {
> > >     ssize_t i_ret;
> > >     i_size += i_size & 1;
> > > 
> > > #if (UINT32_MAX > SSIZE_MAX)
> > >     if( unlikely( i_size > SSIZE_MAX ) )
> > >     {
> > >         i_ret = vlc_stream_Read( p_demux->s, NULL, SSIZE_MAX );
> > >         if( i_ret < 0 || i_ret != SSIZE_MAX )
> > >             return VLC_EGENERIC;
> > >         i_size -= SSIZE_MAX;
> > >         assert( i_size <= SSIZE_MAX );
> > 
> > In other words, assert(i_size <= SIZE_MAX - 1); at function entry.
> > Are you sure this is valid?
> > 
> > >     }
> > > #endif
> > > 
> > >     i_ret = vlc_stream_Read( p_demux->s, NULL, i_size );
> > >     return i_ret < 0 || i_ret != i_size ? VLC_EGENERIC : VLC_SUCCESS;
> > > }
> > > 
> > > uint32_t instead of size_t since value come directly from GetDWLE()
> > 
> > So I guess it is NOT a valid assertion.
> > 
> > > I won't handle uint64_t skip since the 64bit data will never be skipped
> > 
> > Using stream_Read to skip a variable and potentially large block of data is 
> > just a bad idea. Realistically, if the stream is not seekable, you don't want 
> > to read that data to skip it as it could take hours.
> > 
> > IMO, stream_Read(NULL) looks like a convenient hack, but it's just poor 
> > design. We should have a separate function for relative seeking.
> > 
> > -- 
> > レミ・デニ-クールモン
> > http://www.remlab.net/
> > 
> > 
> > 
> > _______________________________________________
> > vlc-devel mailing list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel