[MPlayer-dev-eng] [PATCH] Bluray support through libbluray

[Reimar Döffinger]

On Sat, Jul 03, 2010 at 02:42:37PM +0200, Benjamin Zores wrote:
> Index: stream/stream_bd.c

The other patch already uses that name, please chose a different one,
also for the URL syntax.
Examples:
stream_libbd
stream_br
(I guess bd is supposed to stand for "BluRay Disc", given that this
code actually acts on file level, the bd name is bad anyway...)

> +static void bd_stream_close(stream_t *s)
> +{
> +    BLURAY *bd = s->priv;

All-uppercase is an exceptionally horrible naming convention.

> +    bd_close(bd);

Also s->priv is void *, so this intermediate variable is quite pointless.
And didn't you forget to also free it? Looking at the later code
probably not, but setting it to NULL might be good.

> +static int bd_stream_seek(stream_t *s, off_t pos)
> +{
> +    BLURAY *bd = s->priv;
> +
> +    if (pos > s->end_pos)
> +        return 0;

Shouldn't bd_seek handle that?

> +    s->pos = bd_seek(bd, pos);

How does bd_seek indicate an error?

> +static int bd_stream_fill_buffer(stream_t *s, char *buf, int len)
> +{
> +    BLURAY *bd = s->priv;
> +    int read_len;
> +
> +    if (s->pos > s->end_pos)
> +        return 0;
> +
> +    read_len = bd_read(bd, buf, len);
> +    s->pos   = bd_tell(bd);

This looks very strange, and should be explained a bit,
why is bd_tell called each time?
Can s->pos jump around randomly?
If yes, that might cause major issues with the cache.

> +    bd = calloc(1, sizeof(BLURAY));
> +
> +    /* find the requested device */
> +    if (p->device)
> +      device = p->device;
> +    else if (bd_device)
> +      device = bd_device;

Inconsistent indentation.

> +    /* open device */
> +    bd = bd_open(device, NULL);

Huh, didn't you just calloc it?

> +    *file_format   = DEMUXER_TYPE_MPEG_TS;

This is not acceptable. Not only is it a layering violation and
thus generally should be avoided, it also makes it impossible to
chose the lavf demuxer IIRC.
Also since there is seeking support there should be no need for
this at all anyway, if removing it causes issues this was just
hiding the real bug.

> +    s->url         = strdup("bd://");

Uh, why is this not the full, proper URL?
Also I think setting this really doesn't belong
in the individual stream implementation expect
for special cases.

> +SRCS_COMMON-$(BLURAY)                += stream/stream_bd.c

LIBBLURAY would be a better name.

> +  --disable-bluray       disable Bluray support [autodetect]
>    --disable-dvdnav       disable libdvdnav [autodetect]
>    --disable-dvdread      disable libdvdread [autodetect]
>    --disable-dvdread-internal  disable internal libdvdread [autodetect]
> @@ -653,6 +654,7 @@
>  _libbs2b=auto
>  _xmms=no
>  _vcd=auto
> +_bd=auto

Please use a consistent name like libbluray.