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

[Diego Biurrun]

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.

> --- 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.

> @@ -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?

> @@ -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.

> @@ -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.

> @@ -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.

> +    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.

> --- 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.

Diego