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

[Diego Biurrun]

On Wed, Oct 22, 2014 at 09:38:36AM +0200, Thomas Guillem wrote:
> On Tue, Oct 21, 2014, at 21:53, Diego Biurrun wrote:
> > On Fri, Oct 17, 2014 at 05:43:36PM +0200, Thomas Guillem wrote:
> > > --- a/src/device.c
> > > +++ b/src/device.c
> > > @@ -710,6 +737,33 @@ static int libc_seek( dvdcss_t dvdcss, int i_blocks )
> > > +    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.

Added to my list of pending cleanups.

> > > @@ -809,6 +863,43 @@ static int libc_read ( dvdcss_t dvdcss, void *p_buffer, int i_blocks )
> > > +    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)

probably

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

We don't have uint32_t in the API (for seek offsets) anywhere else, so
you should either change all places or remain consistent.  Also, off_t
is probably the better choice.


Will you make the changes to the patch or should I?

Diego