[MPlayer-dev-eng] [PATCH] vobsub fix for empty useless streams

[Ulion]

2007/9/15, Nico Sabbi <nicola_sabbi at fastwebnet.it>:
> Ulion wrote:
> > ignored?
> >
> >
>
> the link points to an eastern page that almost no one can read
> and understand, much less use to download the file.

That page ( https://www.shooter.cn/files/?fileid=29025&filename=Gladiator-Subs-oTo
) is utf-8 encoded, so everybody should see the same page. There's a
button in the middle of the page, just click it, there will be 6 links
show up bellow the button. Click any of the 6 links will download same
file, that's the vobsub package to be tested.

The download link is dynamic link, only valid for a few minutes after
click the button, so I can not supply a static link for that. And
sometimes need to input verify code after click the button, it's just
simple numbers certainly you can pass the verify.


> Your patch is mostly ok except for the following stylistic reasons:
>
>
> +int
> +vobsub_get_id_by_index(void *vobhandle, unsigned int index)
>
> ugly: it makes harder to find a function definition with grep,
> instead the following is much better
> +int vobsub_get_id_by_index(void *vobhandle, unsigned int index)

Sorry for just following the old code's style.

>
> here:
> +    for (i = 0; i < vob->spu_streams_size; ++i)
> +        if (vob->spu_streams[i].id && j++ == index)
> +            return i;
>
> I find that hidden j++ ugly. What's wrong with  this?
>
> +    for (i = 0; i < vob->spu_streams_size; ++i)
> +        if (vob->spu_streams[i].id && j == index)
> +            return i;
> +        else j++;

Your fix cause problem, j should only be added when
vob->spu_streams[i].id is not NULL. So if this's ugly, I will give
another correct version.

>
>
> here:
> +vobsub_get_index_by_id(void *vobhandle, int id)
> +{
> +    vobsub_t *vob = (vobsub_t *) vobhandle;
> +    int i, j = 0;
> +    if (vob == NULL || id < 0 || id >= vob->spu_streams_size
> +            || vob->spu_streams[id].id == NULL)
> +        return -1;
> +    for (i = 0; i < id; ++i)
> +        if (vob->spu_streams[i].id)
> +            ++j;
> +    return j;
>
> ugly: j should be initialized before the for()

ok.

>
> If you provide a direct link with the vobsub stream
> I'll commit the fixed patch monday unles someone
> objects || the patch doesn't work

I explained how to download the package in that link, you can have a try.

Fixed version is here.


-- 
Ulion
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: vobsub_empty_sub_fix.txt
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20070915/e06b4a51/attachment.txt>