Re: [libdvdcss-devel] [PATCH 1/1] add dvdcss_open_stream

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
> [email protected]
> https://mailman.videolan.org/listinfo/libdvdcss-devel
_______________________________________________
libdvdcss-devel mailing list
[email protected]
https://mailman.videolan.org/listinfo/libdvdcss-devel