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

[Steve Lhomme]

On 2020-03-13 16:53, Thomas Guillem wrote:
> 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 )

In general constant values are bad in the code. You could use a define 
with a meaningful name (and possibly some comments).

>          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
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>