r32035 - in branches/1.0rc4: . libmpdemux/demux_lavf.c
Author: siretart Date: Mon Aug 30 22:07:34 2010 New Revision: 32035 Log: Always use convergence_duration for subtitles otherwise they will stay on screen forever for ASS subtitles in mkv if no ASS renderer is used. backport r32032 by reimar Modified: branches/1.0rc4/ (props changed) branches/1.0rc4/libmpdemux/demux_lavf.c Modified: branches/1.0rc4/libmpdemux/demux_lavf.c ============================================================================== --- branches/1.0rc4/libmpdemux/demux_lavf.c Mon Aug 30 18:48:52 2010 (r32034) +++ branches/1.0rc4/libmpdemux/demux_lavf.c Mon Aug 30 22:07:34 2010 (r32035) @@ -634,7 +634,10 @@ static int demux_lavf_fill_buffer(demuxe if(pkt.pts != AV_NOPTS_VALUE){ dp->pts=pkt.pts * av_q2d(priv->avfc->streams[id]->time_base); priv->last_pts= dp->pts * AV_TIME_BASE; - if(pkt.flags & PKT_FLAG_KEY && pkt.convergence_duration > 0) + // always set endpts for subtitles, even if PKT_FLAG_KEY is not set, + // otherwise they will stay on screen to long if e.g. ASS is demuxed from mkv + if((ds == demux->sub || (pkt.flags & PKT_FLAG_KEY)) && + pkt.convergence_duration > 0) dp->endpts = dp->pts + pkt.convergence_duration * av_q2d(priv->avfc->streams[id]->time_base); } dp->pos=demux->filepos;
On Mon, Aug 30, 2010 at 10:07:34PM +0200, siretart wrote:
Author: siretart Date: Mon Aug 30 22:07:34 2010 New Revision: 32035
Log: Always use convergence_duration for subtitles otherwise they will stay on screen forever for ASS subtitles in mkv if no ASS renderer is used.
backport r32032 by reimar
Actually I noticed that only fixes non-ASS subtitles in MKV, for ASS subs convergence_duration is not set. We'd have to parse the end time out of the text, but I think that's ridiculous since the demuxer had the value as a number originally and sprintfs it explicitly. Hope the lavf mkv demuxer will soon be improved on that.
On Tue, Aug 31, 2010 at 07:45:29AM +0200, Reimar Döffinger wrote:
On Mon, Aug 30, 2010 at 10:07:34PM +0200, siretart wrote:
Author: siretart Date: Mon Aug 30 22:07:34 2010 New Revision: 32035
Log: Always use convergence_duration for subtitles otherwise they will stay on screen forever for ASS subtitles in mkv if no ASS renderer is used.
backport r32032 by reimar
Actually I noticed that only fixes non-ASS subtitles in MKV, for ASS subs convergence_duration is not set. We'd have to parse the end time out of the text,
Sure ! That's the only way to go.
but I think that's ridiculous since the demuxer had the value as a number originally and sprintfs it explicitly.
No, the demuxer generally don't have the subtitle display duration as a field. Matroska is really an exception here, but avi, nut and others don't have such a field. So you really can't expect to get this value from the demuxer. You can give it a try with the following sample (with -demuxer lavf): http://samples.mplayerhq.hu/avi/mewmew-ssa.avi
Hope the lavf mkv demuxer will soon be improved on that.
It will be improved by not exporting text-only subtitles anymore. There is actually no such thing as text-only subtitle in Matroska. It is actually SubRip formatted subtitles (with html like tags, such as <b>, <i>, etc...). So the mkv demuxer will export this as SubRip event, and thus, won't have to misuse convergence_duration anymore, because the duration will be contained in the SubRip text itself. But hopefully, mplayer won't need to care about this. There will also be a SubRip decoder, which will just output ASS event. So mplayer will simply have to use lavc to decode subtitles packets and be able to display ASS events. Aurel
On Wed, Sep 01, 2010 at 12:02:46AM +0200, Aurelien Jacobs wrote:
On Tue, Aug 31, 2010 at 07:45:29AM +0200, Reimar Döffinger wrote:
On Mon, Aug 30, 2010 at 10:07:34PM +0200, siretart wrote:
Author: siretart Date: Mon Aug 30 22:07:34 2010 New Revision: 32035
Log: Always use convergence_duration for subtitles otherwise they will stay on screen forever for ASS subtitles in mkv if no ASS renderer is used.
backport r32032 by reimar
Actually I noticed that only fixes non-ASS subtitles in MKV, for ASS subs convergence_duration is not set. We'd have to parse the end time out of the text,
Sure ! That's the only way to go.
Will you implement it? Because honestly I am a bit annoyed that you almost attack people who try to improve the mkv demuxer while several things are _still_ broken with lavf and I seem to be the only one trying to make it work.
but I think that's ridiculous since the demuxer had the value as a number originally and sprintfs it explicitly.
No, the demuxer generally don't have the subtitle display duration as a field. Matroska is really an exception here, but avi, nut and others don't have such a field.
That doesn't make it less silly to have to do a full sprintf->sscanf roundtrip when the value we want is already there.
On Wed, Sep 01, 2010 at 08:05:27AM +0200, Reimar Döffinger wrote:
On Wed, Sep 01, 2010 at 12:02:46AM +0200, Aurelien Jacobs wrote:
On Tue, Aug 31, 2010 at 07:45:29AM +0200, Reimar Döffinger wrote:
On Mon, Aug 30, 2010 at 10:07:34PM +0200, siretart wrote:
Author: siretart Date: Mon Aug 30 22:07:34 2010 New Revision: 32035
Log: Always use convergence_duration for subtitles otherwise they will stay on screen forever for ASS subtitles in mkv if no ASS renderer is used.
backport r32032 by reimar
Actually I noticed that only fixes non-ASS subtitles in MKV, for ASS subs convergence_duration is not set. We'd have to parse the end time out of the text,
Sure ! That's the only way to go.
Will you implement it?
Probably yes. But only after I get the ASS and SubRip decoder in libavcodec (currently working on it) and that I make use of them in av_sub.c. The end_time parsing might be implemented in av_sub.c (I haven't checked yet where is the best place).
Because honestly I am a bit annoyed that you almost attack people who try to improve the mkv demuxer while several things are _still_ broken with lavf and I seem to be the only one trying to make it work.
Huh ??? Didn't I worked on removing the MAX_STREAMS limit in lavf ? And I'm also working on implementing ASS and SubRip decoder in lavc so that all "text" subtitle comming from lavf (whatever container it comes from) arrive in one single common format (AVSubtitle) to mplayer. Are you aware of anything else broken in lavf (mkv related) ?
but I think that's ridiculous since the demuxer had the value as a number originally and sprintfs it explicitly.
No, the demuxer generally don't have the subtitle display duration as a field. Matroska is really an exception here, but avi, nut and others don't have such a field.
That doesn't make it less silly to have to do a full sprintf->sscanf roundtrip when the value we want is already there.
So are you saying that every single lavf user should implement this if (lavf->subtitle_duration) duration = lavf->subtitle_duration; else sscanf("...", &duration); instead of just this ? sscanf("...", &duration); If so, I guess we have a different definition of what's silly... I think one of the goal of lavf is to give access to all kind of media formats under one common API. Providing a different API for every format supported by lavf, would make it a hell to use. Aurel
On Wed, Sep 01, 2010 at 04:42:02PM +0200, Aurelien Jacobs wrote:
On Wed, Sep 01, 2010 at 08:05:27AM +0200, Reimar Döffinger wrote:
Will you implement it?
Probably yes. But only after I get the ASS and SubRip decoder in libavcodec (currently working on it) and that I make use of them in av_sub.c. The end_time parsing might be implemented in av_sub.c (I haven't checked yet where is the best place).
Huh? If you use av_sub, the subtitle decoder certainly _must_ fill in sub.end_display_time, so why would you need to parse them?
Are you aware of anything else broken in lavf (mkv related) ?
There is a roundup issue on a webm test stream, but I haven't even verified it is even valid.
No, the demuxer generally don't have the subtitle display duration as a field. Matroska is really an exception here, but avi, nut and others don't have such a field.
That doesn't make it less silly to have to do a full sprintf->sscanf roundtrip when the value we want is already there.
So are you saying that every single lavf user should implement this
if (lavf->subtitle_duration) duration = lavf->subtitle_duration; else sscanf("...", &duration);
instead of just this ?
sscanf("...", &duration);
If so, I guess we have a different definition of what's silly...
No, I think that every application having to do the sscanf is silly. Other formats might not already have the duration, but those could e.g. use a parser to generate them. However leaving the whole thing to a ASS decoder instead would be ok as well. However, implementing "ASS parser take 3" in MPlayer isn't reasonable. If a FFmpeg ASS decoder doesn't work for us I guess reusing the subreader code would be reasonable as well (but not really good though).
On Wed, Sep 01, 2010 at 06:22:27PM +0200, Reimar Döffinger wrote:
Are you aware of anything else broken in lavf (mkv related) ?
There is a roundup issue on a webm test stream, but I haven't even verified it is even valid.
Actually not webm, it is issue2177
On Wed, Sep 01, 2010 at 06:22:27PM +0200, Reimar Döffinger wrote:
On Wed, Sep 01, 2010 at 04:42:02PM +0200, Aurelien Jacobs wrote:
On Wed, Sep 01, 2010 at 08:05:27AM +0200, Reimar Döffinger wrote:
Will you implement it?
Probably yes. But only after I get the ASS and SubRip decoder in libavcodec (currently working on it) and that I make use of them in av_sub.c. The end_time parsing might be implemented in av_sub.c (I haven't checked yet where is the best place).
Huh? If you use av_sub, the subtitle decoder certainly _must_ fill in sub.end_display_time, so why would you need to parse them?
Oh, yes. I also thought about this at some point and then forgot about it. This is probably the place to set end_time.
Are you aware of anything else broken in lavf (mkv related) ?
There is a roundup issue on a webm test stream, but I haven't even verified it is even valid.
Thanks for pointing to the issue and showing a possible fix !
No, the demuxer generally don't have the subtitle display duration as a field. Matroska is really an exception here, but avi, nut and others don't have such a field.
That doesn't make it less silly to have to do a full sprintf->sscanf roundtrip when the value we want is already there.
So are you saying that every single lavf user should implement this
if (lavf->subtitle_duration) duration = lavf->subtitle_duration; else sscanf("...", &duration);
instead of just this ?
sscanf("...", &duration);
If so, I guess we have a different definition of what's silly...
No, I think that every application having to do the sscanf is silly.
I didn't say to the application, I said to lavf user. The demuxed stream should obviously be feed to lavc, not directly to the player.
However leaving the whole thing to a ASS decoder instead would be ok as well.
That's what I had in mind.
However, implementing "ASS parser take 3" in MPlayer isn't reasonable.
IMHO MPlayer shouldn't contain any single bit of ASS handling. Subtitle tracks should be decoded by lavc and rendered by lavfi... But the road is still long before we get there...
If a FFmpeg ASS decoder doesn't work for us I guess reusing the subreader code would be reasonable as well (but not really good though).
The (upcoming) decoder has to work for MPlayer (and any other player) or will have to be improved. The point of lavc is to be reused everywhere, so it has to fit all kind of usage including MPlayer. Aurel
On Wed, Sep 01, 2010 at 11:36:47PM +0200, Aurelien Jacobs wrote:
On Wed, Sep 01, 2010 at 06:22:27PM +0200, Reimar Döffinger wrote:
Are you aware of anything else broken in lavf (mkv related) ?
There is a roundup issue on a webm test stream, but I haven't even verified it is even valid.
Thanks for pointing to the issue and showing a possible fix !
Unfortunately the patch I sent does not fix anything at all :-(. Well, at least it now detects it as unknown length, but it still fails to parse it.
However, implementing "ASS parser take 3" in MPlayer isn't reasonable.
IMHO MPlayer shouldn't contain any single bit of ASS handling. Subtitle tracks should be decoded by lavc and rendered by lavfi...
Well, decoding by lavc might happen in a reasonable time, but I wouldn't wait for lavfi :-) Maybe you already looked at it, but it might be worth looking at the SSA handling in subreader.c for the ASS -> text conversion. While I'm sure it has its issues it still is well-tested code (though GPL, so maybe I shouldn't recommend looking at it for writing a libavcodec implementation...).
participants (3)
-
Aurelien Jacobs -
Reimar Döffinger -
siretart