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

[Ulion]

Reimar,

Do you have any further suggestion/objection on this patch, or anybody
else? If none, I will apply this patch after a week.

2007/9/19, Ulion <ulion2002 at gmail.com>:
> 2007/9/19, Reimar Döffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de>:
> > 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.
>
> Fixed, please check it.
>
> >
> > > +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.
>
> Fixed.
>
> >
> > [...]
> > > @@ -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.
>
> Fixed, if -vobsubid is setted and less than vob->spu_streams_size,
> make it visible when loop subtitles.
>
> >
> > > @@ -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).
>
> Since there's no compile warning without the case, removed.
>
> >
> > > +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.
>
> The last condition is removed now to make vobsub stream set by
> -vobsubid can be looped.
>
> >
> > [...]
> > >      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.
>
> Fixed.
>
> Here's the fixed patch.
>
> --
> Ulion
>
>


-- 
Ulion