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

[Reimar Döffinger]

Hello,
On Sat, Sep 15, 2007 at 08:01:16PM +0800, Ulion wrote:
> +/* get vobsub id by it's index in the valid streams */

Comments should be doxygen compatible.

> +extern int vobsub_get_id_by_index(void * /* vobhandle */, unsigned int /* index */);

Stupidity in the surrounding code does not mean you should copy it.
Please don't comment out the parameter names, just write them there
normally.

[...]
> @@ -1165,8 +1167,11 @@
>  		    }
>  		}
>  		vob->spu_streams_current = vob->spu_streams_size;
> -		while (vob->spu_streams_current-- > 0)
> +		while (vob->spu_streams_current-- > 0) {
>  		    vob->spu_streams[vob->spu_streams_current].current_index = 0;
> +		    if (vob->spu_streams[vob->spu_streams_current].id)
> +			++vob->spu_valid_streams_size;
> +		}

This is potentially the big Achilles' heel of this patch: It assumes
that we know all subtitle tracks on open. This it seems relies on a
correct .idx file. Maybe this requirement is ok, though I'd be happy if
someone could comment on it conclusively.
But even then, it should be possible to select any stream with -vobsubid
even when it is not in the .idx or .ifo or whatever file.

> @@ -1201,6 +1206,34 @@
>      return (index < vob->spu_streams_size) ? vob->spu_streams[index].id : NULL;
>  }
>  
> +int vobsub_get_id_by_index(void *vobhandle, unsigned int index)
> +{
> +    vobsub_t *vob = (vobsub_t *) vobhandle;

Useless cast (and I am aware that the surrounding code is full of
those).

> +int vobsub_get_index_by_id(void *vobhandle, int id)
> +{
> +    vobsub_t *vob = (vobsub_t *) vobhandle;
> +    int i, j;
> +    if (vob == NULL || id < 0 || id >= vob->spu_streams_size
> +            || vob->spu_streams[id].id == NULL)

?? That last condition I don't understand.

[...]
>      if (source == SUB_SOURCE_VOBSUB) {
> -	vobsub_id =
> -	    mpctx->global_sub_pos -
> -	    mpctx->global_sub_indices[SUB_SOURCE_VOBSUB];
> +        vobsub_id = vobsub_get_id_by_index(vo_vobsub, mpctx->global_sub_pos -
> +            mpctx->global_sub_indices[SUB_SOURCE_VOBSUB]);

While I don't like them, the surrounding code uses tabs, so please leave
them in.

Greetings,
Reimar Döffinger