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

[Jonathan Nell]

2009/9/13 Reimar Döffinger <Reimar.Doeffinger at gmx.de>:
> On Sun, Sep 13, 2009 at 04:51:01PM +0300, Jonathan Nell wrote:

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

Ok - it's on the TODO list.

>> +    s->priv = (void *)bd;
>
> The cast is still completely pointless.


Of course - forgot about this

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

Yeah, but a rounded position makes sense in this context because
decryption happens in BD_UNIT_SIZE blocks. Can't really give mplayer
the exact position it's seeking to. of course s->pos needs to be
correct as you mention.

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

Decrypting into buf might be neater but is inefficient as the same
unit will then be decrypted three times. The trade off is an extra 6k
in mem...

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

Indeed.

>> +    FILE *file = NULL;

Agreed.

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

Proper checking isn't implemented yet - I just wanted to get something
functional out for testing. Will hopefully implement tomorrow.
The max size of this file is 64k, it is only read into mem once. I'm
not sure it's worth making this any more complex. In the current
implementation, I don't use this file for much, but when I implement
from processing key (pk) step, this will be needed.

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

Hmm.. will give this some more thought. In most cases there is only 1
unit key (uk) so this would only be performed once. I think I can
neaten this up at some point though. There isn't a really a need to
store the encrypted key as you say - this I'll change tomorrow.

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

I'll take a look at how ugly it looks tomorrow.

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

Could do, yeah.

>> +    // 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;

Ok. I like mod but your solution is faster.

> And all functions lack doxygen comments of course.

TODO

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

Fair enough.

>> +#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 []

Nothing wrong with the preprocessor dealing with these (BD_CBC_IV
being an exception). I think it's just a style thing.

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

Yup.

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

Yup.