[MPlayer-dev-eng] [PATCH] Initial Bluray support

[Reimar Döffinger]

On Sun, Sep 13, 2009 at 04:51:01PM +0300, Jonathan Nell wrote:
> Thanks to you both. I have implemented most of the changes suggested.
> Please see new version attached.
> 
> 2009/9/13 Reimar Döffinger <Reimar.Doeffinger at gmx.de>:
> >> +  // change vuk ascii hex into byte array
> >> +  for(i=0;i<16;++i) {
> >> +    if(sscanf(p->vuk,"%2x",&byte)!=1) break;
> >> +    bd->vuk[i]=byte;
> >> +    p->vuk+=2;
> >> +  }
> >
> > libmpdemux/demux_lavf already has a function for that.
> 
> Where? All I can see is parse_cryptokey() which works on AVFormatContext.

It can be changed. Two different implementations simply lead to
inconsistent behaviour usually.

> +    s->sector_size = BD_UNIT_SIZE;
> +    s->flags = STREAM_READ | STREAM_SEEK;
> +    s->fill_buffer = bd_stream_fill_buffer;
> +    s->seek = bd_stream_seek;
> +    s->close = bd_stream_close;
> +    s->start_pos = 0;
> +    s->priv = (void *)bd;

The cast is still completely pointless.

> +static int bd_stream_seek(stream_t *s, off_t pos)
> +{
> +    bd_priv_t *bd = s->priv;
> +
> +    bd->cur_unit_buf_part = 0;
> +    bd->cur_unit = pos / BD_UNIT_SIZE;

I have some doubts that this fits the API, I'd think MPlayer expects
seeking to seek to the position it requested, not to some "rounded"
position.
Well, it actually should work, but you'd at least have to set s->pos
correctly (I admit at least the DVD code does some really strange
things...).

> +static int bd_stream_fill_buffer(stream_t *s, char *buf, int len)
> +{
> +    bd_priv_t *bd = s->priv;
> +
> +    if(bd->cur_unit_buf_part == 0)
> +        bd_read_unit(bd, bd->unit_buf);
> +
> +    memcpy(buf, &bd->unit_buf[len*bd->cur_unit_buf_part], 2048);

What? What is len*bd->cur_unit_buf_part supposed to calculate?
Also at least as long as you assume that buf, pos and len are divisible
by 16 it should be easy enough to decrypt directly into buf.
Or to make it simpler just assume a multiple of 2048 since that's what
the code currently does anyway (as well as the DVD code).

> +    return len;

This is just wrong, both when len != 2048 and when a read error occurred
or EOF was reached.

> +    FILE *file = NULL;

Pointless initialization

> +    file = fopen(filename, "rb");
> +    fseek(file, 0, SEEK_END);

Will crash if filename does not exist.

> +    buf = malloc(file_size);
> +    fread(buf, 1, file_size, file);

Will crash if malloc failed. Also reading the whole file into memory is
ugly and in addition it probably even needs more complex code.

> +    pos = AV_RB32(buf);
> +    if(pos < file_size) {
> +        bd->uks.count = AV_RB16(&buf[pos]);
> +        bd->uks.keys = calloc(bd->uks.count,sizeof(struct uk));
> +
> +        for(i=0; i<bd->uks.count; i++) {
> +          int key_pos = pos + i * 48 + 48;
> +          if(key_pos + 16 <= file_size) {
> +              memcpy(bd->uks.keys[i].enc_key,&buf[key_pos],16);
> +
> +              // dec unit key
> +              a = av_malloc(av_aes_size);
> +              av_aes_init(a, bd->vuk ,128 ,1);
> +              av_aes_crypt(a, bd->uks.keys[i].dec_key, bd->uks.keys[i].enc_key, 1, NULL, 1);
> +              free(a);

Obviously the whole malloc/av_aes_init/free cycle should only be done
once per VUK.
I am also not sure if there is a point in storing the raw uks keys
instead of the av_aes_init result.

> +    unsigned char enc_seed[16];
> +    unsigned char iv[16];

uint64_t would mean you don't need a loop for the xor and it is faster,
though you might needs some extra casts (or use a about as ugly union).

> +    a = av_malloc(av_aes_size);
[...]
> +    free(a);
[...]
> +    a=av_malloc(av_aes_size);
[...]
> +    free(a);

It's rather obvious this is silly, right?
You should malloc once an keep it in the priv struct.

> +    // mark packets as decrypted
> +    for(i=0; i<BD_UNIT_SIZE; i++)
> +        if(!(i%192) || !i)
> +            buf[i] &= 0x3f;

0%192 == 0, so "if (!(i%192))" is the same thing.
But mod is slow, so it should be
for (i = 0; i < BD_UNIT_SIZE; i += 192) buf[i] &= 0x3f;


And all functions lack doxygen comments of course.

> Index: stream/stream_bd.h
> ===================================================================
> --- stream/stream_bd.h	(revision 0)
> +++ stream/stream_bd.h	(revision 0)

I can't see the point of this header.

> +#define BD_UNIT_SIZE 	6144
> +#define BD_CBC_IV 		"\x0B\xA0\xF8\xDD\xFE\xA6\x1F\xB3\xD8\xDF\x9F\x56\x6A\x05\x0F\x78"
> +#define BD_UKF_PATH 	"/%s/AACS/Unit_Key_RO.inf"
> +#define BD_MKB_PATH 	"/%s/AACS/MKB_RO.inf"
> +#define BD_M2TS_PATH 	"/%s/BDMV/STREAM/%05ld.m2ts"

I have some doubts these should be defines, particularly
BD_CBC_IV should be a uint8_t []

> +static int bd_stream_open(stream_t *s, int mode, void* opts, int* file_format);
> +static void bd_stream_close(stream_t *s);
> +static int bd_stream_seek(stream_t *s, off_t pos);
> +static int bd_stream_fill_buffer(stream_t *s, char *buf, int len);

Static functions in a header without their implementations are nonsense,
they only work in the file where those functions are defined - so there
is no point in not just putting the functions in the C file in the right
order instead.

> +off_t bd_read_unit(bd_priv_t *bd, unsigned char *buf);
> +
> +void bd_get_uks(bd_priv_t *bd);
> +void bd_dec_unit(bd_priv_t *bd, unsigned char *buf);

I doubt there is much of a point to make those non-static at the current
point.