[PATCH 1/2] lavf: set codec time base after finding streams.
For most formats, it is done before reading packets, but for formats without global headers, new streams are added afterwards. Ensure that a time base is set even if no decoder was found. Useful for parsers. Signed-off-by: Nicolas George <nicolas.george@normalesup.org> --- libavformat/utils.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/libavformat/utils.c b/libavformat/utils.c index 0f13304..903ada3 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -2947,6 +2947,14 @@ int avformat_find_stream_info(AVFormatContext *ic, AVDictionary **options) compute_chapters_end(ic); + for(i = 0; i < ic->nb_streams; i++) { + st = ic->streams[i]; + if ((st->codec->codec_type == AVMEDIA_TYPE_VIDEO || + st->codec->codec_type == AVMEDIA_TYPE_SUBTITLE) && + !st->codec->time_base.num) + st->codec->time_base = st->time_base; + } + find_stream_info_err: for (i=0; i < ic->nb_streams; i++) { if (ic->streams[i]->codec) -- 1.7.10.4
Set the packet duration according to the times in it. Signed-off-by: Nicolas George <nicolas.george@normalesup.org> --- libavcodec/dvdsub_parser.c | 51 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) Without this patch (and the previous one, since otherwise the time base is not set), ffmpeg -i foo.vob -c copy bar.mkv will not work because the subtitles packets do not have their duration; mkvalidator considers the file invalid and mkvmerge does not see the tracks. As a side note, I would very much like to understand how the PTS is supposed to work with parsers, since lavf assumes the PTS are in stream time base but the time base is not available. diff --git a/libavcodec/dvdsub_parser.c b/libavcodec/dvdsub_parser.c index e50c339..e451bdc 100644 --- a/libavcodec/dvdsub_parser.c +++ b/libavcodec/dvdsub_parser.c @@ -37,6 +37,55 @@ static av_cold int dvdsub_parse_init(AVCodecParserContext *s) return 0; } +static void parse_duration(AVCodecParserContext *s, AVCodecContext *avctx) +{ + DVDSubParseContext *pc = s->priv_data; + uint8_t *buf = pc->packet; + unsigned buf_size = pc->packet_len; + uint8_t *end = buf + buf_size; + unsigned hd = 0, offset_size = 2, cmd_pos = 0, new_pos, date, pos, cmd; + unsigned time_start = 0, time_end = 0; + int64_t ts_start, ts_end; + AVRational tb = { 1, 90000 }, ctb = avctx->time_base; + + if (buf_size < 10) + return; + if (!AV_RB16(buf)) { /* HD, untested */ + hd = 1; + offset_size = 4; + cmd_pos += 4; + } + while (1) { + new_pos = hd ? AV_RB32(buf + cmd_pos + 2) : AV_RB16(buf + cmd_pos + 2); + if (new_pos <= cmd_pos || new_pos >= buf_size - 2 - offset_size) + break; + cmd_pos = new_pos; + date = AV_RB16(buf + cmd_pos); + pos = cmd_pos + 2 + offset_size; + while (pos < buf_size) { + cmd = buf[pos++]; + switch (cmd) { + case 0x01: time_start = date << 10; break; + case 0x02: time_end = date << 10; break; + case 0x00: break; + case 0x03: + case 0x04: pos += 2; break; + case 0x05: + case 0x85: pos += 6; break; + case 0x06: pos += 4; break; + case 0x86: pos += 8; break; + case 0x83: pos += 768; break; + case 0x84: pos += 256; break; + default: pos = buf_size; break; + } + } + } + ts_start = av_rescale_q_rnd(time_start, tb, ctb, AV_ROUND_DOWN); + ts_end = av_rescale_q_rnd(time_end, tb, ctb, AV_ROUND_DOWN); + /* FIXME: update pts, but its time base is not available */ + s->duration = ts_end - ts_start; +} + static int dvdsub_parse(AVCodecParserContext *s, AVCodecContext *avctx, const uint8_t **poutbuf, int *poutbuf_size, @@ -61,6 +110,8 @@ static int dvdsub_parse(AVCodecParserContext *s, *poutbuf = pc->packet; *poutbuf_size = pc->packet_len; pc->packet_index = 0; + if (avctx->time_base.num && avctx->time_base.den) + parse_duration(s, avctx); return buf_size; } } else { -- 1.7.10.4
On Fri, Dec 07, 2012 at 05:58:45PM +0100, Nicolas George wrote:
Set the packet duration according to the times in it.
Signed-off-by: Nicolas George <nicolas.george@normalesup.org> --- libavcodec/dvdsub_parser.c | 51 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+)
Without this patch (and the previous one, since otherwise the time base is not set), ffmpeg -i foo.vob -c copy bar.mkv will not work because the subtitles packets do not have their duration; mkvalidator considers the file invalid and mkvmerge does not see the tracks.
As a side note, I would very much like to understand how the PTS is supposed to work with parsers, since lavf assumes the PTS are in stream time base but the time base is not available.
diff --git a/libavcodec/dvdsub_parser.c b/libavcodec/dvdsub_parser.c index e50c339..e451bdc 100644 --- a/libavcodec/dvdsub_parser.c +++ b/libavcodec/dvdsub_parser.c
this patch seems to still apply cleanly, what is its status ? [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I know you won't believe me, but the highest form of Human Excellence is to question oneself and others. -- Socrates
On 12/6/2013 12:10 AM, Michael Niedermayer wrote:
this patch seems to still apply cleanly, what is its status ?
This problem keeps popping up in #matroska. Is there anything I can do to help move this patch along? Ping Nicolas. - Derek
Le septidi 7 pluviôse, an CCXXII, Derek Buitenhuis a écrit :
This problem keeps popping up in #matroska.
Is there anything I can do to help move this patch along?
I need to dig back into it to remember what it is about and what needs to be done; this is not very high in my list of priorities, but I can raise it a bit. If you want to take it over, do not hesitate. If not, maybe you can say a few words about what is said about it on #matroska? Regards, -- Nicolas George
On 1/26/2014 6:50 PM, Nicolas George wrote:
I need to dig back into it to remember what it is about and what needs to be done; this is not very high in my list of priorities, but I can raise it a bit.
OK.
If you want to take it over, do not hesitate.
Maybe...
If not, maybe you can say a few words about what is said about it on #matroska?
[04:20] < fundies> [greg@greg-desktop 15]$ mkvextract tracks dvd.mkv 2:vobsub [04:20] < fundies> Error: Track 2 with the CodecID 'S_VOBSUB' is missing the "codec private" element and cannot be extracted. [04:20] < fundies> any way to extract the vobsubs? [...] [09:39] < Mosu> fundies: Not with mkvextract, no. [09:40] < Mosu> You could simply patch that part out and re-compile, of course. [...] [20:08] < fundies> Mosu, cant you just fix it upstream? Shouldn't it really be a warning not an abort mission? [20:11] < Mosu> fundies: Well, it's not VobSub without the CodecPrivate stuff. That's the whole .idx file of the .idx/.sub pair. [20:13] < Mosu> Though I wouldn't reject patches that turn this into a warning and skip writing the .idx. The warning would have to be clear why the .idx isn't created. [20:17] < fundies> you must be able to generate an idx from just the .sub somehow as players know what time to display the subs without them [20:22] < Mosu> That's not the problem; the timestamps are indeed part of the Matroska cluster/block structure. They're therefore not stored in CodecPrivate. So creating the .sub without CodecPrivate is perfectly possible. But the .idx part contains information that simply is not available in the .sub part, and if it's not present in CodecPrivate then it cannot be created. If you look at an .idx file then everything but the "timestamp:" lines is what's unique to ... [20:22] < Mosu> ... the .idx file, and that information is simply not recreatable. Yes, you can play the .sub without the palette information etc. But I haven't seen one legitimate file that does this. [20:22] < Mosu> Anyway, like I said, I'm open to patches. I just won't do it myself as I have no such files and currently no interest in spending the time on it. Sorry. [20:23] < fundies> I'm not a programmer so I couldn't fix it [20:24] < fundies> also ffmpeg always generates files like this [20:24] < Mosu> Then don't use ffmpeg. [20:24] < fundies> I have to [20:25] < fundies> dgindex causes desync [20:25] < Mosu> I've already spent eight hours coding today, and I seriously am not in the business of granting everybody's wishes. So I'm sorry, but no. [20:43] < Rodeo> someone should really fix ffmpeg then [20:43] < Daemon404> ... or even report it [20:43] < Rodeo> if it actually doesn't write CodecPrivate for VobSub [20:43] < Rodeo> yeah, that too [20:43] < fundies> it is reported and theres a patch [20:43] < Rodeo> fundies: surely you can do that, if you have a reproducible test case? [20:43] < fundies> but is been ignored [20:43] < Daemon404> fundies, link [20:43] < Daemon404> i will try and push it through [20:43] < Rodeo> ah, where is the report please [20:44] < fundies> h/o lemme find it [20:44] < wm4> ffmpeg generates what? [20:44] < wm4> vobsubs without codec private? [20:44] < Daemon404> apparently [20:44] < fundies> http://trac.ffmpeg.org/ticket/2035 [20:44] < wm4> how evil [20:45] < wm4> oh I see [20:45] < wm4> remuxing from VOB [20:45] < Daemon404> fundies, added to my TODO for tomorrow [20:45] < wm4> I don't know much about this stuff, but I'm not sure if ffmpeg can even get the "codec private" from this? [20:45] < wm4> e.g. libdvdread returns this information separately from the mpeg stream [...] [20:47] < Rodeo> well, we could create a "default" palette, which might be considered better than no palette at all - Derek
On Sun, Jan 26, 2014 at 8:44 PM, Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote:
On 1/26/2014 6:50 PM, Nicolas George wrote:
I need to dig back into it to remember what it is about and what needs to be done; this is not very high in my list of priorities, but I can raise it a bit.
OK.
If you want to take it over, do not hesitate.
Maybe...
If not, maybe you can say a few words about what is said about it on #matroska?
[04:20] < fundies> [greg@greg-desktop 15]$ mkvextract tracks dvd.mkv 2:vobsub [04:20] < fundies> Error: Track 2 with the CodecID 'S_VOBSUB' is missing the "codec private" element and cannot be extracted. [04:20] < fundies> any way to extract the vobsubs?
[...]
[09:39] < Mosu> fundies: Not with mkvextract, no. [09:40] < Mosu> You could simply patch that part out and re-compile, of course.
[...]
[20:08] < fundies> Mosu, cant you just fix it upstream? Shouldn't it really be a warning not an abort mission? [20:11] < Mosu> fundies: Well, it's not VobSub without the CodecPrivate stuff. That's the whole .idx file of the .idx/.sub pair. [20:13] < Mosu> Though I wouldn't reject patches that turn this into a warning and skip writing the .idx. The warning would have to be clear why the .idx isn't created. [20:17] < fundies> you must be able to generate an idx from just the .sub somehow as players know what time to display the subs without them [20:22] < Mosu> That's not the problem; the timestamps are indeed part of the Matroska cluster/block structure. They're therefore not stored in CodecPrivate. So creating the .sub without CodecPrivate is perfectly possible. But the .idx part contains information that simply is not available in the .sub part, and if it's not present in CodecPrivate then it cannot be created. If you look at an .idx file then everything but the "timestamp:" lines is what's unique to ... [20:22] < Mosu> ... the .idx file, and that information is simply not recreatable. Yes, you can play the .sub without the palette information etc. But I haven't seen one legitimate file that does this. [20:22] < Mosu> Anyway, like I said, I'm open to patches. I just won't do it myself as I have no such files and currently no interest in spending the time on it. Sorry. [20:23] < fundies> I'm not a programmer so I couldn't fix it [20:24] < fundies> also ffmpeg always generates files like this [20:24] < Mosu> Then don't use ffmpeg. [20:24] < fundies> I have to [20:25] < fundies> dgindex causes desync [20:25] < Mosu> I've already spent eight hours coding today, and I seriously am not in the business of granting everybody's wishes. So I'm sorry, but no. [20:43] < Rodeo> someone should really fix ffmpeg then [20:43] < Daemon404> ... or even report it [20:43] < Rodeo> if it actually doesn't write CodecPrivate for VobSub [20:43] < Rodeo> yeah, that too [20:43] < fundies> it is reported and theres a patch [20:43] < Rodeo> fundies: surely you can do that, if you have a reproducible test case? [20:43] < fundies> but is been ignored [20:43] < Daemon404> fundies, link [20:43] < Daemon404> i will try and push it through [20:43] < Rodeo> ah, where is the report please [20:44] < fundies> h/o lemme find it [20:44] < wm4> ffmpeg generates what? [20:44] < wm4> vobsubs without codec private? [20:44] < Daemon404> apparently [20:44] < fundies> http://trac.ffmpeg.org/ticket/2035 [20:44] < wm4> how evil [20:45] < wm4> oh I see [20:45] < wm4> remuxing from VOB [20:45] < Daemon404> fundies, added to my TODO for tomorrow [20:45] < wm4> I don't know much about this stuff, but I'm not sure if ffmpeg can even get the "codec private" from this? [20:45] < wm4> e.g. libdvdread returns this information separately from the mpeg stream
[...]
[20:47] < Rodeo> well, we could create a "default" palette, which might be considered better than no palette at all
- Derek
Just my 2 cents, just judging from the ffmpeg decoder implementation: Its actually better to let it "guess" the palette based on the encoded frames, then to try to feed it some default palette. It can apply more smarts based on which colors are actually used to guess the palette, than any plain default ever could. Of course that doesn't solve the muxing problem.
Le septidi 7 pluviôse, an CCXXII, Derek Buitenhuis a écrit :
[04:20] < fundies> [greg@greg-desktop 15]$ mkvextract tracks dvd.mkv 2:vobsub [04:20] < fundies> Error: Track 2 with the CodecID 'S_VOBSUB' is missing the "codec private" element and cannot be extracted. [04:20] < fundies> any way to extract the vobsubs?
[...]
[09:39] < Mosu> fundies: Not with mkvextract, no. [09:40] < Mosu> You could simply patch that part out and re-compile, of course.
[...]
[20:08] < fundies> Mosu, cant you just fix it upstream? Shouldn't it really be a warning not an abort mission? [20:11] < Mosu> fundies: Well, it's not VobSub without the CodecPrivate stuff. That's the whole .idx file of the .idx/.sub pair. [20:13] < Mosu> Though I wouldn't reject patches that turn this into a warning and skip writing the .idx. The warning would have to be clear why the .idx isn't created. [20:17] < fundies> you must be able to generate an idx from just the .sub somehow as players know what time to display the subs without them [20:22] < Mosu> That's not the problem; the timestamps are indeed part of the Matroska cluster/block structure. They're therefore not stored in CodecPrivate. So creating the .sub without CodecPrivate is perfectly possible. But the .idx part contains information that simply is not available in the .sub part, and if it's not present in CodecPrivate then it cannot be created. If you look at an .idx file then everything but the "timestamp:" lines is what's unique to ... [20:22] < Mosu> ... the .idx file, and that information is simply not recreatable. Yes, you can play the .sub without the palette information etc. But I haven't seen one legitimate file that does this. [20:22] < Mosu> Anyway, like I said, I'm open to patches. I just won't do it myself as I have no such files and currently no interest in spending the time on it. Sorry. [20:23] < fundies> I'm not a programmer so I couldn't fix it [20:24] < fundies> also ffmpeg always generates files like this [20:24] < Mosu> Then don't use ffmpeg. [20:24] < fundies> I have to [20:25] < fundies> dgindex causes desync [20:25] < Mosu> I've already spent eight hours coding today, and I seriously am not in the business of granting everybody's wishes. So I'm sorry, but no. [20:43] < Rodeo> someone should really fix ffmpeg then [20:43] < Daemon404> ... or even report it [20:43] < Rodeo> if it actually doesn't write CodecPrivate for VobSub [20:43] < Rodeo> yeah, that too [20:43] < fundies> it is reported and theres a patch [20:43] < Rodeo> fundies: surely you can do that, if you have a reproducible test case? [20:43] < fundies> but is been ignored [20:43] < Daemon404> fundies, link [20:43] < Daemon404> i will try and push it through [20:43] < Rodeo> ah, where is the report please [20:44] < fundies> h/o lemme find it [20:44] < wm4> ffmpeg generates what? [20:44] < wm4> vobsubs without codec private? [20:44] < Daemon404> apparently [20:44] < fundies> http://trac.ffmpeg.org/ticket/2035 [20:44] < wm4> how evil [20:45] < wm4> oh I see [20:45] < wm4> remuxing from VOB [20:45] < Daemon404> fundies, added to my TODO for tomorrow [20:45] < wm4> I don't know much about this stuff, but I'm not sure if ffmpeg can even get the "codec private" from this? [20:45] < wm4> e.g. libdvdread returns this information separately from the mpeg stream
Thanks for the excerpt. I suspect this is unrelated to this patch, though. This discussion is about the IDX file / codec extradata. IIRC, FFmpeg handles it correctly, but the required information is frequently not present: dvdsub come from MPEG-PS VOB streams, but the palette information and the rest of the IDX file are in the IFO file of DVD-video structures. As long as FFmpeg does not support libdvdread / libdvdnav, there is not much that can be done. This patch addresses the packet duration problem: packet in MPEG-PS VOB have no duration, parsing the packets is necessary to determine it. Duration is mandatory on Matroska subtitles packets. Regards, -- Nicolas George
On Fri, Dec 07, 2012 at 05:58:44PM +0100, Nicolas George wrote:
For most formats, it is done before reading packets, but for formats without global headers, new streams are added afterwards.
Ensure that a time base is set even if no decoder was found. Useful for parsers.
this is problematic, because streams can be added still even after this point. IMHO for cases where this is needed, the demuxer should set it up correctly, thats of course not the only way by which it could be done [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Avoid a single point of failure, be that a person or equipment.
participants (5)
-
Derek Buitenhuis -
Hendrik Leppkes -
Michael Niedermayer -
Nicolas George -
Nicolas George