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

[Reimar Döffinger]

On Mon, Sep 14, 2009 at 11:50:53AM +0300, Jonathan Nell wrote:
> +#include <malloc.h>

I don't think this is needed, if it is, it needs to be under ifdef.

> +#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"

Should use static const variables instead of defines. Also BD_CBC_IV clearly
is not a string, so it should use the {} syntax not this pseudo-string.

> +static struct stream_priv_s {
> +    int title;
> +    char* device;
> +    unsigned char* vuk;

uint8_t if the sign is relevant.
However since this is a normal string I'm almost certain that just "char"
would be the right type.

> +struct uk {
> +    unsigned char key[16];
> +};

A struct is rather overkill for that.

> +    unsigned long title;

int unless there's a good reason to use something else.

> +    unsigned char vuk[16];

uint8_t

> +    FILE* title_file;

Inconsistent * placement

> +    unsigned char iv[16];

uint8_t

> +    unsigned char *buf;

uint8_t

> +    size_t file_size;

off_t + a check to limit the size to something sensible would make
more sense IMO.

> +    int pos;

uint32_t

> +    char filename[200];

MAX_PATH/PATH_MAX whatever it is called may be a better choice.

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

The first file_size assignment is pointless.

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

Only reading exactly what we need would probably even
result in smaller and simpler code, but certainly
in more robust code.

> +          int key_pos = pos + i * 48 + 48;
> +          if (key_pos + 16 <= file_size)
> +              av_aes_crypt(a, bd->uks.keys[i].key, &buf[key_pos], 1, NULL, 1);	// decrypt unit key

I doubt buf + key_pos is guaranteed to match the alignment requirements.
Reading only what is necessary and using av_malloc for the read buffer
would probably avoid this without any extra code.

> +static off_t bd_read(struct bd_priv *bd, unsigned char *buf, int len)
> +{
> +	int read_len;

Tab. Many more tabs in that function.

> +    if ( (read_len = fread(buf, 1, len, bd->title_file)) == len) {

Don't do assignments in ifs. It has not purpose unless it's an entry for
the obfuscated C contest or similar.

> +    	    unsigned char enc_seed[16];

I think there might be alignment issues here as well.
At least for now, declaring it as uint64_t instead should be
enough for now, though not the most beautiful solution.

> +       	    a = av_malloc(av_aes_size);

This should be allocated at most once and stored in the context,
one malooc + free per block read is too much.

> +            av_aes_init(a, bd->uks.keys[0].key, 128, 0);

And if it always uses the key the init should be done only once, too.
init is a possibly quite slow function.

> +    char filename[200];

Same as for other filename array.