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

[Thomas Guillem]

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