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

[Benjamin Zores]

On Sat, Jul 3, 2010 at 3:49 PM, Reimar Döffinger
<Reimar.Doeffinger at gmx.de> wrote:
> 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...)

I really don't like it much because the other patch:
1. doesn't exists yet
2. clearly has no future
3. there will be a bdnav:// (using libbluray too) as a complement to bd://

So what would be the best naming convention ?

I went for bluray:// (or blu://) in attached patch.
Though libbluray continuously mixes references to bluray and bd.

>> +static void bd_stream_close(stream_t *s)
>> +{
>> +    BLURAY *bd = s->priv;
>
> All-uppercase is an exceptionally horrible naming convention.

100% agree but well, that's how libbluray exports its structure. I
can't do much here.

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

free() is done in bd_close() (I know it's ugly ...). Doing free() here
results in segv/ due to double-free.
Ok to avoid the cast.

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

Seems it does, simplified it.

>
>> +    s->pos = bd_seek(bd, pos);
>
> How does bd_seek indicate an error?

Returns -1, fixed.

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

You're right and bd_tell() is useless here so removed.

> Inconsistent indentation.

Fixed.

>> +    /* open device */
>> +    bd = bd_open(device, NULL);
>
> Huh, didn't you just calloc it?

Fixed.

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

I did it as to speed up format detection but it can be removed as
being auto-detection.
FYI though, DVD streaming code does the same (but with MPEG-PS obviously).

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

Same goes for DVD streaming code.

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

Done.
See new patch.

Ben
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mplayer-bluray-support-2.diff
Type: text/x-diff
Size: 15947 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20100703/ec002232/attachment.diff>