[libdvdcss-devel] [PATCH 1/1] add dvdcss_open_stream
Thomas Guillem
tom at gllm.fr
Wed Oct 22 09:38:36 CEST 2014
On Tue, Oct 21, 2014, at 21:53, Diego Biurrun wrote:
> On Fri, Oct 17, 2014 at 05:43:36PM +0200, Thomas Guillem wrote:
> > open a DVD Device using external read/seek callbacks.
> > ---
> > src/device.c | 114 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > src/dvdcss/dvdcss.h | 12 ++++++
> > src/libdvdcss.c | 33 ++++++++++++++-
> > src/libdvdcss.h | 3 ++
> > 4 files changed, 161 insertions(+), 1 deletion(-)
>
> This needs a NEWS entry.
Ok
>
> > --- a/src/device.c
> > +++ b/src/device.c
> > @@ -200,6 +207,10 @@ void _dvdcss_check ( dvdcss_t dvdcss )
> >
> > + /* If stream is set, return */
> > + if( dvdcss->p_stream )
> > + return;
> > +
> > /* If the device name is non-null, return */
> > if( dvdcss->psz_device[0] )
> > {
>
> These two if-blocks could be merged.
Ok
>
> > @@ -358,6 +369,22 @@ int _dvdcss_open ( dvdcss_t dvdcss )
> >
> > + /* if callback functions are initialized */
> > + if( dvdcss->p_stream )
> > + {
> > +#if defined( WIN32 )
> > + /* Initialize readv temporary buffer */
> > + dvdcss->p_readv_buffer = NULL;
> > + dvdcss->i_readv_buf_size = 0;
> > +#endif
>
> Why? This initialization looks unnecessary and misplaced.
>
> It's done in the '#if WIN32' block below and seems only relevant for the
> WIN32, not the stream, case. What am I missing?
This is remainder of David patch, I don't know why too, I'll test on
windows without that part.
>
> > @@ -710,6 +737,33 @@ static int libc_seek( dvdcss_t dvdcss, int i_blocks )
> >
> > +static int stream_seek( dvdcss_t dvdcss, int i_blocks )
> > +{
> > + off_t i_seek;
>
> nit: odd spacing
>
> > + i_seek = (off_t)i_blocks * (off_t)DVDCSS_BLOCK_SIZE;
> > +
> > + if( dvdcss->p_stream_cb->pf_seek( dvdcss->p_stream, i_seek ) != 0 )
> > + {
> > + print_error( dvdcss, "seek error" );
> > + dvdcss->i_pos = -1;
> > + return -1;
> > + }
> > +
> > + dvdcss->i_pos = i_seek / DVDCSS_BLOCK_SIZE;
>
> Just use i_blocks instead of recalculating the value of i_blocks.
Agree, but I just did like libc_seek.
>
> > @@ -809,6 +863,43 @@ static int libc_read ( dvdcss_t dvdcss, void *p_buffer, int i_blocks )
> >
> > +static int stream_read ( dvdcss_t dvdcss, void *p_buffer, int i_blocks )
> > +{
> > + off_t i_size, i_ret;
> > +
> > + if( !dvdcss->p_stream_cb->pf_read )
> > + return -1;
> > +
> > + i_size = (off_t)i_blocks * (off_t)DVDCSS_BLOCK_SIZE;
>
> same comment about the casts
>
> > + i_ret = dvdcss->p_stream_cb->pf_read( dvdcss->p_stream, p_buffer, i_size );
> > +
> > + if( i_ret < 0 )
> > + {
> > + print_error( dvdcss, "read error" );
> > + dvdcss->i_pos = -1;
> > + return i_ret;
> > + }
> > +
> > + /* Handle partial reads */
> > + if( i_ret != i_size )
> > + {
> > + int i_seek;
> > +
> > + dvdcss->i_pos = -1;
> > + i_seek = stream_seek( dvdcss, i_ret / DVDCSS_BLOCK_SIZE );
> > +
> > + /* We have to return now so that i_pos isn't clobbered */
> > + return i_ret / DVDCSS_BLOCK_SIZE;
> > + }
> > +
> > + dvdcss->i_pos += i_ret / DVDCSS_BLOCK_SIZE;
> > + return i_ret / DVDCSS_BLOCK_SIZE;
> > +}
>
> This is so similar to libc_read, I wonder if the code duplication cannot
> be avoided.
Yes it is. I wonder also if we should left this logic to the code that
implements stream_cb.
(and have dvdcss->pf_seek = stream_cb->pf_seek) (see my comment just
below)
>
> > @@ -911,6 +1002,29 @@ static int libc_readv ( dvdcss_t dvdcss, struct iovec *p_iovec, int i_blocks )
> >
> > +/*****************************************************************************
> > + * stream_readv: vectored read
> > + *****************************************************************************/
> > +static int stream_readv ( dvdcss_t dvdcss, struct iovec *p_iovec, int i_blocks )
> > +{
> > + int i_read;
> > +
> > + if( !dvdcss->p_stream_cb->pf_readv )
> > + return -1;
> > +
>
> trailing whitespace
>
> > + i_read = dvdcss->p_stream_cb->pf_readv( dvdcss->p_stream, p_iovec,
> > + i_blocks );
>
> odd indentation
>
> > --- a/src/dvdcss/dvdcss.h
> > +++ b/src/dvdcss/dvdcss.h
> > @@ -38,6 +40,14 @@ extern "C" {
> >
> > +struct dvdcss_stream_cb
> > +{
> > + int ( *pf_seek ) ( void *p_stream, uint64_t i_pos);
>
> Why uint64_t all of a sudden? That signature does not match the
> functions
> you assign IIRC.
It's an error on the functions, I wanted a uint64_t for seek offset.
I don't really know which is the better: uint64_t i_pos or int i_blocks.
If we use the last, the code implementing the stream_cb must know the
size of a block.
>
> > + int ( *pf_read ) ( void *p_stream, void* buffer, int i_read);
>
> void *buffer
>
> > + int ( *pf_readv ) ( void *p_stream, void *p_iovec, int i_blocks);
> > +};
> > +typedef struct dvdcss_stream_cb dvdcss_stream_cb;
>
> For dvdcss_t we typedef a pointer.
Ah yes, ok with that.
>
> > --- a/src/libdvdcss.c
> > +++ b/src/libdvdcss.c
> > @@ -136,6 +136,8 @@
> >
> > +static dvdcss_t dvdcss_open_common ( char *psz_target, void *p_stream,
> > + dvdcss_stream_cb *p_stream_cb );
>
> const char *
>
> > @@ -152,6 +154,27 @@
> > */
> > LIBDVDCSS_EXPORT dvdcss_t dvdcss_open ( char *psz_target )
>
> OK, I guess we need to fix that here first ..
>
> > +/**
> > + * \brief Open a DVD device using dvdcss_stream_cb
>
> .
>
> > @@ -164,6 +187,10 @@ LIBDVDCSS_EXPORT dvdcss_t dvdcss_open ( char *psz_target )
> >
> > + if( psz_target == NULL &&
> > + ( p_stream == NULL || p_stream_cb == NULL ) )
> > + return NULL;
> > +
> > @@ -180,13 +207,17 @@ LIBDVDCSS_EXPORT dvdcss_t dvdcss_open ( char *psz_target )
> > dvdcss->p_titles = NULL;
> > - dvdcss->psz_device = (char *)strdup( psz_target );
> > + dvdcss->psz_device = psz_target != NULL ?
> > + (char *)strdup( psz_target ) : NULL;
>
> You already performed this check a few lines above.
ah yes.
>
> Diego
> _______________________________________________
> libdvdcss-devel mailing list
> libdvdcss-devel at videolan.org
> https://mailman.videolan.org/listinfo/libdvdcss-devel
More information about the libdvdcss-devel
mailing list