[vlc-devel] [RFC PATCH] demux: subtitle: workaround out-of-order samples

[Filip Roséen]

Hi Zhao,

On 2018-09-04 14:57, Zhao Zhili wrote:

> ---
> A test sample:
> https://drive.google.com/open?id=11kQP2OrbFyPnmbH9BPS__kJzbsXKmAQT
> 
> I totally understand it may not worth to support such broken files. In
> this case, it looks like a small effort. The time takes by Fix() is
> less than 200 us on my PC with the sample.
> 
>  modules/demux/subtitle.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/modules/demux/subtitle.c b/modules/demux/subtitle.c
> index 9af186e..5e64ba4 100644
> --- a/modules/demux/subtitle.c
> +++ b/modules/demux/subtitle.c
> @@ -696,7 +696,10 @@ static int Open ( vlc_object_t *p_this )
>          fmt.subs.cc.i_reorder_depth = -1;
>      }
>      else
> +    {
> +        Fix( p_demux );
>          es_format_Init( &fmt, SPU_ES, VLC_CODEC_SUBT );
> +    }

I don't know how many samples there are in the wild where the order of
individual subtitles are not in increasing timestamp order, and
theoretically sorting the parsed entities is not a big thing (as you
have mentioned), but the location for this fix hurts my eyes a little.

As it is definitely possible to detect out-of-order subtitles during
parsing, *if* we want such an explicit invocation of `Fix` I vote that
one flags the necessity of that during parsing, and then invoke the
function later if that is needed.

Adding a call to `Fix` where your patch does also results in a rather
weird looking *if-else-tree* (given that a call to `Fix` already
exists in there).

While you are at it, you could also fix the issue of `p_sys->i_length`
being inaccurate, as it is populated with the timestamp of the last
subtitle, even if said input is later `Fix`ed (potentially resulting
in the last subtitle not at all being the last subtitle).

>  
>      /* Stupid language detection in the filename */
>      char * psz_language = get_language_from_filename( p_demux->psz_filepath );

Best Regards,
Filip
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20180920/7044ebcc/attachment.html>