[vlc-devel] [PATCH 2/2] DASH: implement Seek()
Hugo Beauzée-Luyssen
beauze.h at gmail.com
Fri Mar 23 14:26:07 CET 2012
Hello,
Comments inline.
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
> +{
> + 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.
> + 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.
> + {
> + 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?
> 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.
> + 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
> + 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??
> + {
> + uint64_t pos = (uint64_t)va_arg(args, uint64_t);
> + if(Seek(p_stream, pos) == VLC_SUCCESS)
> + {
> + p_sys->position = pos;
> + break;
> + }
> + }
> return VLC_EGENERIC;
> case STREAM_GET_SIZE:
> {
> --
> 1.7.5.4
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> http://mailman.videolan.org/listinfo/vlc-devel
Regards,
--
Hugo Beauzée-Luyssen
More information about the vlc-devel
mailing list