[PATCH] Title key cache and minor fixes

[Adam Jones]

Hi all.

I've now split the title key cache patch into two - a bugfix patch and a
patch containing only the cache functionality (which must be applied on
top of the first). The bugfix patch is attached, since it's pretty
trivial and should be non-controversial.

Both patches are available at:

http://www.yggdrasl.demon.co.uk/libdvdcss/

which seems a sensible place to put them for now.

[H}kan Hjort]

Post by Adam Jones
Hi all.
I've now split the title key cache patch into two - a bugfix patch and a
patch containing only the cache functionality (which must be applied on
top of the first). The bugfix patch is attached, since it's pretty
trivial and should be non-controversial.

(Removed the typos as obvious).


diff -u --recursive libdvdcss-1.2.1/src/libdvdcss.c libdvdcss-1.2.1-fixed/src/libdvdcss.c
--- libdvdcss-1.2.1/src/libdvdcss.c Sun Jun 2 16:54:10 2002
+++ libdvdcss-1.2.1-fixed/src/libdvdcss.c Wed Jun 26 20:51:38 2002
@@ -300,6 +300,7 @@
{
dvd_title_t *p_title;
dvd_title_t *p_newtitle;
+ dvd_title_t *p_firsttitle;
dvd_key_t p_title_key;
int i_ret;

@@ -342,8 +343,9 @@
}

/* Find our spot in the list */
p_newtitle = NULL;
p_title = dvdcss->p_titles;
+ p_firsttitle = p_title;
while( ( p_title != NULL ) && ( p_title->i_startlb < i_block ) )
{
p_newtitle = p_title;
@@ -357,10 +359,16 @@
memcpy( p_newtitle->p_key, p_title_key, KEY_SIZE );

/* Link the new title, either at the beginning or inside the list */
- if( p_title == NULL )
+ if( p_firsttitle == NULL )
{
dvdcss->p_titles = p_newtitle;
p_newtitle->p_next = NULL;
+ }
+ else if (p_title == NULL)
+ {
+ p_newtitle->p_next = p_firsttitle;
+ p_firsttitle->p_next = NULL;
+ dvdcss->p_titles = p_newtitle;
}
else
{

What are you trying to fix here? A small description of how it fails
whould have made it much simpler to know if this was the right fix.
Is this for the case of insering a key first in a non empty list?

[a***@yggdrasl.demon.co.uk]

***@dtek.chalmers.se wrote:

<snip diffs>

Post by H}kan Hjort
What are you trying to fix here? A small description of how it fails
whould have made it much simpler to know if this was the right fix.
Is this for the case of insering a key first in a non empty list?

Sorry... <slaps self> Would help if I'd said. Yes, you're correct.
When inserting a key at the start of a non-empty list, the first list
item was being lost (but not freed). This would lead to:

1) a minor memory leak (as all those "lost" first entries got dropped
each time a seek was done back to an early disc block)
2) lots and lots of repeated entries in the title key cache (since
DVD playing seems to re-read early blocks a lot in menus, etc.)

#2 was what alerted me to the problem in the first place. My apologies
for the poor (non-existent) explanation.

Would it be helpful for me to post a better explanation of the cache
patch to the list?

[H}kan Hjort]

Post by a***@yggdrasl.demon.co.uk
<snip diffs>

Post by H}kan Hjort
What are you trying to fix here? A small description of how it fails
whould have made it much simpler to know if this was the right fix.
Is this for the case of insering a key first in a non empty list?

Sorry... <slaps self> Would help if I'd said. Yes, you're correct.
When inserting a key at the start of a non-empty list, the first list
1) a minor memory leak (as all those "lost" first entries got dropped
each time a seek was done back to an early disc block)
2) lots and lots of repeated entries in the title key cache (since
DVD playing seems to re-read early blocks a lot in menus, etc.)
#2 was what alerted me to the problem in the first place. My apologies
for the poor (non-existent) explanation.

Ok, I rewrote it a bit. No need to add that extra if.
(Hope I didn't break it again)

Post by a***@yggdrasl.demon.co.uk
Would it be helpful for me to post a better explanation of the cache
patch to the list?

I haven't had time to look at it yet but sure, if it's not to much work.
It's always easier with some comments about what the intended purposes
is of the patches, rather than having to guess why things are as they
are.

[a***@yggdrasl.demon.co.uk]

Post by H}kan Hjort

Post by a***@yggdrasl.demon.co.uk
When inserting a key at the start of a non-empty list, the first list
item was being lost (but not freed).

Ok, I rewrote it a bit. No need to add that extra if.

Right. I thought there was probably a way, but it just didn't spring to
mind whilst I was writing.

Post by H}kan Hjort

Post by a***@yggdrasl.demon.co.uk
Would it be helpful for me to post a better explanation of the cache
patch to the list?

I haven't had time to look at it yet but sure, if it's not to much work.

Okay. The cache patch is rather longer, but the idea is pretty simple
(although the code is probably a bit naive - this is my first time
sending patches to a net-based project).

In summary, we're essentially building a database of CSS title keys
keyed by disc serial number and block number.


The patch adds two local routines to libdvdcss.c:
_dvdcss_cache_init() and _dvdcss_get_serial()

It also adds two new members to the dvdcss_handle struct to keep track of
the cache file.

In _dvdcss_open() the environment variable DVDCSS_USE_CACHE is checked.
If true, then the directory $HOME/.dvdcss is created to hold key cache
files (name defined in libdvdcss.h). If the disc is encrypted, then
_dvdcss_cache_init() is called.

_dvdcss_cache_init() calls _dvdcss_get_serial(), which returns a
serial number string for the disc (the concept for this code came
from libdvdread) - this serial number is used as a unique filename
for the cache file. (Problem: what happens when playing individual
vob files rather than DVD discs? Someone with more knowledge of the
format of .vob files please help.)

The cache file is then read, simply populating the cache list in the
dvdcss_handle structure.

Finally, a change is made to dvdcss_title() such that when a key is
cracked (as opposed to read from the memory cache list), it is appended
to the cache file in the form:

block: 0xbyte0, 0xbyte1, 0xbyte2, 0xbyte3, 0xbyte4


I've been using this for a couple of weeks now on a range of discs
and it seems to work fine for me. However, it will probably need
some work to make it less Linux-specific (one or two calls changing
is all, I think) and just general cleanups.

(Apologies for any iffy coding. This is a learning experience for me
as much as anything...)

[H}kan Hjort]

Post by a***@yggdrasl.demon.co.uk

Post by H}kan Hjort

Post by a***@yggdrasl.demon.co.uk
When inserting a key at the start of a non-empty list, the first list
item was being lost (but not freed).

Ok, I rewrote it a bit. No need to add that extra if.

Right. I thought there was probably a way, but it just didn't spring to
mind whilst I was writing.

This is what I checked in.

Index: src/libdvdcss.c
===================================================================
RCS file: /var/cvs/videolan/libdvdcss/src/libdvdcss.c,v
retrieving revision 1.11
diff -p -u -d -r1.11 libdvdcss.c
--- src/libdvdcss.c 2002/06/04 07:02:57 1.11
+++ src/libdvdcss.c 2002/07/01 13:38:55
@@ -337,7 +337,7 @@ extern int dvdcss_title ( dvdcss_handle
}
else if( i_ret == 0 )
{
- _dvdcss_debug( dvdcss, "unecrypted title" );
+ _dvdcss_debug( dvdcss, "unencrypted title" );
/* Still store this in the cache, so we don't need to check again. */
}

@@ -358,12 +358,13 @@ extern int dvdcss_title ( dvdcss_handle
p_newtitle->i_startlb = i_block;
memcpy( p_newtitle->p_key, p_title_key, KEY_SIZE );

- /* Link the new title, either at the beginning or inside the list */
+ /* Link it at the head of the (possibly empty) list */
if( p_title == NULL )
{
+ p_newtitle->p_next = dvdcss->p_titles;
dvdcss->p_titles = p_newtitle;
- p_newtitle->p_next = NULL;
}
+ /* Link the new title inside the list */
else
{
p_newtitle->p_next = p_title->p_next;

[a***@yggdrasl.demon.co.uk]

Post by H}kan Hjort
This is what I checked in.

[...]

Post by H}kan Hjort
+ p_newtitle->p_next = dvdcss->p_titles;
dvdcss->p_titles = p_newtitle;
- p_newtitle->p_next = NULL;

Ah. That looks far more sensible - thanks. I'll modify the code in
_dvdcss_cache_init() in the other patch to do the same.