[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