[MPlayer-dev-eng] [RFC] subtitle cleanup

[Reimar Doeffinger]

Hello,
On Fri, Dec 22, 2006 at 02:13:05AM +0200, Uoti Urpala wrote:
> On Thu, 2006-12-21 at 23:26 +0100, Reimar Döffinger wrote:
> > Improved version with less code in mplayer.c and a lot of code in
> > demux_mkv.c replaced by shared code in sub.c.
> 
> I don't particularly like using libvo/sub.c for this. It has nothing to
> do with vo. Combining it with find_sub.c/subreader.c would be a better
> match IMO.

Since it is all about setting vo_sub it has all to do with vo.

> +  vo_sub = &mkv_d->subs;
> +  vo_sub_add_text(ptr1, strlen(ptr1),
> +                  (timecode + block_duration) / 1000.0f);
> +
>    sub_utf8 = 1;
>  #ifdef USE_ASS
>    if (ass_enabled) {
>      mkv_d->subs.start = timecode / 10;
>      mkv_d->subs.end = (timecode + block_duration) / 10;
>      ass_process_subtitle(track->sh_sub.ass_track, &mkv_d->subs);
> -  } else
> +  }
>  #endif
> -  vo_sub = &mkv_d->subs;
> -  vo_osd_changed (OSDTYPE_SUBTITLE);
> 
> This looks wrong, now if libass rendering for non-SSA subtitles is
> enabled they'd be rendered twice. Either the libass rendering case
> should be moved from here to the sub_add_text function (probably the
> better choice to also support it for other demuxers, though subs without
> duration immediately given wouldn't work with current API) or
> sub_add_text should not be called when ass_enabled is set.

Actually I think the clear_subtitle from above should just be moved into
the ass_enabled (even without this patch I think this would make more
sense, at least if it works, I did not test)

> +    char type = d_dvdsub->sh ? ((sh_sub_t *)d_dvdsub->sh)->type : 'v';
> 
> The variable should probably be renamed from d_dvdsub to something else
> if it's not specific to dvd subs any more.

Well, neither is dvdsub_id since a long, long time...

> +        double nextpts = ds_get_next_pts(d_dvdsub);
> 
> As I explained on IRC using current ds_get_next_pts for this is wrong.

And I still think that fixing the demuxers to handle the problems with
this is better than implementing yet another API that AFAICS will add a
lot in terms complexity but little in new functionality.

> Also as is the patch would cause a regression as it converts demux_mkv.c
> to use the new code even though it doesn't yet implement multiple
> simultaneous subtitles.

This patch does support that, if not it's a bug. It does not yet support
duration when passing the subtitle messages via demuxer packets, but
even that should not require much more than adding a duration field to the
packet structure.

Greetings,
Reimar Döffinger