[vlc-devel] [PATCH 2/2] DASH: implement Seek()

Frederic YHUEL fyhuel at viotech.net
Fri Mar 23 16:00:09 CET 2012


On Fri, Mar 23, 2012 at 2:26 PM, Hugo Beauzée-Luyssen
<beauze.h at gmail.com> wrote:
> Hello,
>
> Comments inline.
>

Thanks for the review!

> On Thu, Mar 15, 2012 at 2:01 PM,  <fyhuel at viotech.net> wrote:
>> From: Frédéric Yhuel <fyhuel at viotech.net>
>>
>> This is needed for DASH module to work with (not yet ready) VLC MP4
>> demux. Forward seeking is implemented by calling Read() with a NULL
>> argument (= skipping), while backward seeking is limited to the current
>> block in the block_bytestream_t upon which is built the internal buffer.
>> This limitation is not really a problem because backward seeking will be
>> needed only for the parsing of the initialization fragment.
>> ---
>>  modules/stream_filter/dash/DASHManager.cpp        |    5 +++
>>  modules/stream_filter/dash/DASHManager.h          |    7 ++--
>>  modules/stream_filter/dash/buffer/BlockBuffer.cpp |   16 ++++++++
>>  modules/stream_filter/dash/buffer/BlockBuffer.h   |   17 +++++----
>>  modules/stream_filter/dash/dash.cpp               |   41 +++++++++++++++++++++
>>  5 files changed, 75 insertions(+), 11 deletions(-)
>>
>> diff --git a/modules/stream_filter/dash/DASHManager.cpp b/modules/stream_filter/dash/DASHManager.cpp
>> index a98e01a..bbe418d 100644
>> --- a/modules/stream_filter/dash/DASHManager.cpp
>> +++ b/modules/stream_filter/dash/DASHManager.cpp
>> @@ -75,6 +75,11 @@ int     DASHManager::read( void *p_buffer, size_t len )
>>     return this->buffer->get(p_buffer, len);
>>  }
>>
>> +int     DASHManager::seek_backwards( size_t i_len )
> This kinda breaks the naming scheme. More importantly, you are using a
> size_t here, and an unsigned in BlockBuffer::seek_backward
>

Ok

>> +{
>> +    return this->buffer->seek_backwards( i_len );
>> +}
>> +
>>  int     DASHManager::peek( const uint8_t **pp_peek, size_t i_peek )
>>  {
>>     return this->buffer->peek(pp_peek, i_peek);
>> diff --git a/modules/stream_filter/dash/DASHManager.h b/modules/stream_filter/dash/DASHManager.h
>> index 6c9d16a..8c9f199 100644
>> --- a/modules/stream_filter/dash/DASHManager.h
>> +++ b/modules/stream_filter/dash/DASHManager.h
>> @@ -45,9 +45,10 @@ namespace dash
>>                          logic::IAdaptationLogic::LogicType type, stream_t *stream);
>>             virtual ~DASHManager    ();
>>
>> -            bool    start   ();
>> -            int     read    ( void *p_buffer, size_t len );
>> -            int     peek    ( const uint8_t **pp_peek, size_t i_peek );
>> +            bool    start          ();
>> +            int     read           ( void *p_buffer, size_t len );
>> +            int     seek_backwards ( size_t len );
>> +            int     peek           ( const uint8_t **pp_peek, size_t i_peek );
>>
>>             const mpd::IMPDManager*         getMpdManager   () const;
>>             const logic::IAdaptationLogic*  getAdaptionLogic() const;
>> diff --git a/modules/stream_filter/dash/buffer/BlockBuffer.cpp b/modules/stream_filter/dash/buffer/BlockBuffer.cpp
>> index 056c481..56fa73f 100644
>> --- a/modules/stream_filter/dash/buffer/BlockBuffer.cpp
>> +++ b/modules/stream_filter/dash/buffer/BlockBuffer.cpp
>> @@ -85,6 +85,22 @@ int     BlockBuffer::peek                 (const uint8_t **pp_peek, unsigned int
>>     vlc_mutex_unlock(&this->monitorMutex);
>>     return ret;
>>  }
>> +int     BlockBuffer::seek_backwards       (unsigned len)
>> +{
>> +    vlc_mutex_lock(&this->monitorMutex);
>
> You could use the vlc_mutex_locker helper.
>

What and where is that? And mutexes are used this way in the rest of
the file, so we should be consistent, no?

>> +    if( this->buffer.i_offset > len )
>> +    {
>> +        this->buffer.i_offset -= len;
>> +        this->sizeBytes += len;
>> +        vlc_mutex_unlock(&this->monitorMutex);
>> +        return VLC_SUCCESS;
>> +    }
>> +    else
> The else isn't really necessary.
>

Ok

>> +    {
>> +        vlc_mutex_unlock(&this->monitorMutex);
>> +        return VLC_EGENERIC;
>> +    }
>> +}
>>  int     BlockBuffer::get                  (void *p_data, unsigned int len)
>>  {
>>     vlc_mutex_lock(&this->monitorMutex);
>> diff --git a/modules/stream_filter/dash/buffer/BlockBuffer.h b/modules/stream_filter/dash/buffer/BlockBuffer.h
>> index 570861f..b7ae6cd 100644
>> --- a/modules/stream_filter/dash/buffer/BlockBuffer.h
>> +++ b/modules/stream_filter/dash/buffer/BlockBuffer.h
>> @@ -46,14 +46,15 @@ namespace dash
>>                 BlockBuffer           (stream_t *stream);
>>                 virtual ~BlockBuffer  ();
>>
>> -                void    put         (block_t *block);
>> -                int     get         (void *p_data, unsigned int len);
>> -                int     peek        (const uint8_t **pp_peek, unsigned int i_peek);
>> -                void    setEOF      (bool value);
>> -                bool    getEOF      ();
>> -                mtime_t size        ();
>> -                void    attach      (IBufferObserver *observer);
>> -                void    notify      ();
>> +                void    put            (block_t *block);
>> +                int     get            (void *p_data, unsigned int len);
>> +                int     seek_backwards (unsigned len);
>> +                int     peek           (const uint8_t **pp_peek, unsigned int i_peek);
>> +                void    setEOF         (bool value);
>> +                bool    getEOF         ();
>> +                mtime_t size           ();
>> +                void    attach         (IBufferObserver *observer);
>> +                void    notify         ();
>>
> Could you split the cosmetic part in another commit?
>

Ok

>>             private:
>>                 mtime_t             capacityMicroSec;
>> diff --git a/modules/stream_filter/dash/dash.cpp b/modules/stream_filter/dash/dash.cpp
>> index 98123c0..0543887 100644
>> --- a/modules/stream_filter/dash/dash.cpp
>> +++ b/modules/stream_filter/dash/dash.cpp
>> @@ -160,6 +160,38 @@ static void Close(vlc_object_t *p_obj)
>>  /*****************************************************************************
>>  * Callbacks:
>>  *****************************************************************************/
>> +
>> +static int  Seek            ( stream_t *p_stream, uint64_t pos )
>> +{
>> +    stream_sys_t        *p_sys          = (stream_sys_t *) p_stream->p_sys;
>> +    dash::DASHManager   *p_dashManager  = p_sys->p_dashManager;
>> +    int                 i_ret           = 0;
>> +    size_t              i_len           = 0;
>> +    long                i_read          = 0;
>> +
>> +    if( pos < (unsigned)p_sys->position )
> position should be changed to uint64_t as well I guess.
>
>> +    {
>> +        i_len = p_sys->position - pos;
>> +        msg_Dbg( p_stream, "I'm gonna try to seek %u bytes backward!", i_len );
> This should probably be removed.
>

Ok

>> +        i_ret = p_dashManager->seek_backwards( i_len );
>> +        if( i_ret == VLC_EGENERIC )
>> +        {
>> +            msg_Err( p_stream, "Cannot seek backward outside the current block :-/" );
>> +            return VLC_EGENERIC;
>> +        }
>> +        else
>> +            return VLC_SUCCESS;
>> +    }
>> +
>> +    /* Seek forward */
>> +    i_len = pos - p_sys->position;
>> +    i_read = Read( p_stream, (void *)NULL, i_len );
>> +    if( (unsigned)i_read == i_len )
> i_read should be cast to size_t
>

Ok

>> +        return VLC_SUCCESS;
>> +    else
>> +        return VLC_EGENERIC;
>> +}
>> +
>>  static int  Read            (stream_t *p_stream, void *p_buffer, unsigned int i_len)
>>  {
>>     stream_sys_t        *p_sys          = (stream_sys_t *) p_stream->p_sys;
>> @@ -220,6 +252,15 @@ static int  Control         (stream_t *p_stream, int i_query, va_list args)
>>             *(va_arg (args, uint64_t *)) = p_sys->position;
>>             break;
>>         case STREAM_SET_POSITION:
>> +            if(true)
> Why??
>

You don't want to know... this is silly of course.

Thanks,

-- 
Frédéric



More information about the vlc-devel mailing list