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

[Zhao Zhili]

Hi Filip,

On 9/20/18 8:59 PM, Filip Roséen wrote:
> 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).
> 

I contacted with the authors of the sample. They made .srt subtitles 
from .ass with a tool which I think is unpopular. They prefer .ass 
format and care little about .srt format. So never mind.

> 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).

Done.
https://mailman.videolan.org/pipermail/vlc-devel/2018-September/121236.html

> 
>     |/* Stupid language detection in the filename */ char * psz_language
>     = get_language_from_filename( p_demux->psz_filepath );|
> 
> Best Regards, Filip
> 
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20180920/ff33f5b9/attachment.html>