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

On Wed, Oct 22, 2014, at 13:23, Diego Biurrun wrote:
> 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.

ah yes, off_t.
So what do we do for stream_cb seek callback ? off_t i_pos or int
i_blocks, and have the stream_cb implementation use DVDCSS_BLOCK_SIZE ?

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

I agree to make all the changes, but if you want to do it because of
your pending other changes, I won't mind.

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