| Message ID | 20200405155928.9323-13-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | 945b92873061c66b82df892887cd9baf8146857b |
| Headers | show |
| Series | Matroska muxer patches | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
On Sun, 5 Apr 2020 at 16:01, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote: > > The Matroska muxer currently only adds CuePoints in three cases: > a) For video keyframes. b) For the first audio frame in a new Cluster if > in DASH-mode. c) For subtitles. This means that ordinary Matroska audio > files won't have any Cues which impedes seeking. > > This commit changes this. For every track in a file without video track > it is checked and tracked whether a Cue entry has already been added > for said track for the current Cluster. This is used to add a Cue entry > for each first packet of each track in each Cluster. > > Implements #3149. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- Fixed at last, danke schön! This was a very annoying bug. One thing I noticed is, however, that the spec recommends "CuePoint Elements SHOULD reference audio keyframes at most once every 500 milliseconds" [1], but this is not checked currently. [1]: https://cellar-wg.github.io/matroska-specification/cues.html
Jan Chren (rindeal): > On Sun, 5 Apr 2020 at 16:01, Andreas Rheinhardt > <andreas.rheinhardt@gmail.com> wrote: >> >> The Matroska muxer currently only adds CuePoints in three cases: >> a) For video keyframes. b) For the first audio frame in a new Cluster if >> in DASH-mode. c) For subtitles. This means that ordinary Matroska audio >> files won't have any Cues which impedes seeking. >> >> This commit changes this. For every track in a file without video track >> it is checked and tracked whether a Cue entry has already been added >> for said track for the current Cluster. This is used to add a Cue entry >> for each first packet of each track in each Cluster. >> >> Implements #3149. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- > > Fixed at last, danke schön! This was a very annoying bug. > > One thing I noticed is, however, that the spec recommends "CuePoint > Elements SHOULD reference audio keyframes at most once every 500 > milliseconds" [1], but this is not checked currently. > > [1]: https://cellar-wg.github.io/matroska-specification/cues.html I actually pondered whether I should opt for a time-based approach (like mkvmerge with its 2s) or for the approach that I implemented here. I chose the latter because with the former one has to perform two seeks when seeking (when one makes use of the CueRelativePosition element) or one has to parse a potentially nonnegligible amount of data from the Cluster. It is the same reason one nowadays starts new Clusters upon seeing a video keyframe. That being said it is very hard to break this limit when using the default values for cluster_size_limit (5 MiB) and cluster_time_limit. You'd need to have 80 Mib/s to do that and that would be very uncommon for non-videoo (even for short spikes). And if the user uses non-default values, then it is because the user explicitly wants to have smaller Clusters and such a user probably wants to have more Cue entries. I don't see a reason why an arbitrary value from the spec should override this (mkvmerge btw also allows to create files that violate this recommendation if the user opts to add Cue entries for every frame). I think it should rather be the recommendation from the spec that should be adapted. - Andreas
> On April 5, 2020 5:59 PM Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote: > > > The Matroska muxer currently only adds CuePoints in three cases: > a) For video keyframes. b) For the first audio frame in a new Cluster if > in DASH-mode. c) For subtitles. This means that ordinary Matroska audio > files won't have any Cues which impedes seeking. > > This commit changes this. For every track in a file without video track > it is checked and tracked whether a Cue entry has already been added > for said track for the current Cluster. This is used to add a Cue entry > for each first packet of each track in each Cluster. > > Implements #3149. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavformat/matroskaenc.c | 21 +++++++++++-------- > tests/ref/fate/aac-autobsf-adtstoasc | 4 ++-- > tests/ref/fate/matroska-flac-extradata-update | 4 ++-- > tests/ref/lavf/mka | 4 ++-- > 4 files changed, 18 insertions(+), 15 deletions(-) > > diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c > index b9bfd34756..a469d48cc0 100644 > --- a/libavformat/matroskaenc.c > +++ b/libavformat/matroskaenc.c > @@ -2120,6 +2120,10 @@ static void mkv_end_cluster(AVFormatContext *s) > MatroskaMuxContext *mkv = s->priv_data; > > end_ebml_master_crc32(s->pb, &mkv->cluster_bc, mkv, MATROSKA_ID_CLUSTER, 0, 1); > + if (!mkv->have_video) { You could initialize it even for video. In case in the future you want a Cue only on the first video keyframe of the Cluster. > + for (unsigned i = 0; i < s->nb_streams; i++) > + mkv->tracks[i].has_cue = 0; > + } > mkv->cluster_pos = -1; > avio_write_marker(s->pb, AV_NOPTS_VALUE, AVIO_DATA_MARKER_FLUSH_POINT); > } > @@ -2222,7 +2226,7 @@ static int mkv_check_new_extra_data(AVFormatContext *s, AVPacket *pkt) > return 0; > } > > -static int mkv_write_packet_internal(AVFormatContext *s, AVPacket *pkt, int add_cue) > +static int mkv_write_packet_internal(AVFormatContext *s, AVPacket *pkt) > { > MatroskaMuxContext *mkv = s->priv_data; > AVIOContext *pb; > @@ -2268,10 +2272,12 @@ static int mkv_write_packet_internal(AVFormatContext *s, AVPacket *pkt, int add_ > ret = mkv_write_block(s, pb, MATROSKA_ID_SIMPLEBLOCK, pkt, keyframe); > if (ret < 0) > return ret; > - if ((s->pb->seekable & AVIO_SEEKABLE_NORMAL) && (par->codec_type == AVMEDIA_TYPE_VIDEO && keyframe || add_cue)) { > + if ((s->pb->seekable & AVIO_SEEKABLE_NORMAL) && keyframe && > + (par->codec_type == AVMEDIA_TYPE_VIDEO || !mkv->have_video && !track->has_cue)) { You may restrict this to Audio/Subtitles. Other streams/tracks may not create a cue entry. > ret = mkv_add_cuepoint(mkv, pkt->stream_index, ts, > mkv->cluster_pos, relative_packet_pos, -1); > if (ret < 0) return ret; > + track->has_cue = 1; > } > } else { > if (par->codec_id == AV_CODEC_ID_WEBVTT) { > @@ -2336,8 +2342,7 @@ static int mkv_write_packet(AVFormatContext *s, AVPacket *pkt) > // on seeing key frames. > start_new_cluster = keyframe; > } else if (mkv->is_dash && codec_type == AVMEDIA_TYPE_AUDIO && > - (mkv->cluster_pos == -1 || > - cluster_time > mkv->cluster_time_limit)) { > + cluster_time > mkv->cluster_time_limit) { Is this related to this patch ? It currently means that if the Cluster did not write any Block then a new cluster is not needed. > // For DASH audio, we create a Cluster based on cluster_time_limit > start_new_cluster = 1; > } else if (!mkv->is_dash && > @@ -2361,9 +2366,7 @@ static int mkv_write_packet(AVFormatContext *s, AVPacket *pkt) > > // check if we have an audio packet cached > if (mkv->cur_audio_pkt.size > 0) { > - // for DASH audio, a CuePoint has to be added when there is a new cluster. > - ret = mkv_write_packet_internal(s, &mkv->cur_audio_pkt, > - mkv->is_dash ? start_new_cluster : 0); > + ret = mkv_write_packet_internal(s, &mkv->cur_audio_pkt); > av_packet_unref(&mkv->cur_audio_pkt); > if (ret < 0) { > av_log(s, AV_LOG_ERROR, > @@ -2378,7 +2381,7 @@ static int mkv_write_packet(AVFormatContext *s, AVPacket *pkt) > if (pkt->size > 0) > ret = av_packet_ref(&mkv->cur_audio_pkt, pkt); > } else > - ret = mkv_write_packet_internal(s, pkt, 0); > + ret = mkv_write_packet_internal(s, pkt); > return ret; > } > > @@ -2406,7 +2409,7 @@ static int mkv_write_trailer(AVFormatContext *s) > > // check if we have an audio packet cached > if (mkv->cur_audio_pkt.size > 0) { > - ret = mkv_write_packet_internal(s, &mkv->cur_audio_pkt, 0); > + ret = mkv_write_packet_internal(s, &mkv->cur_audio_pkt); > if (ret < 0) { > av_log(s, AV_LOG_ERROR, > "Could not write cached audio packet ret:%d\n", ret); > diff --git a/tests/ref/fate/aac-autobsf-adtstoasc b/tests/ref/fate/aac-autobsf-adtstoasc > index f1c6f889d4..d9191fb37f 100644 > --- a/tests/ref/fate/aac-autobsf-adtstoasc > +++ b/tests/ref/fate/aac-autobsf-adtstoasc > @@ -1,5 +1,5 @@ > -9d0c81ce285a84c0137316004d091d95 *tests/data/fate/aac-autobsf-adtstoasc.matroska > -6620 tests/data/fate/aac-autobsf-adtstoasc.matroska > +76a14cc1b3292c7f724006d56b7e2eac *tests/data/fate/aac-autobsf-adtstoasc.matroska > +6648 tests/data/fate/aac-autobsf-adtstoasc.matroska > #extradata 0: 2, 0x0030001c > #tb 0: 1/1000 > #media_type 0: audio > diff --git a/tests/ref/fate/matroska-flac-extradata-update b/tests/ref/fate/matroska-flac-extradata-update > index dfb2851b0f..16b268c4a8 100644 > --- a/tests/ref/fate/matroska-flac-extradata-update > +++ b/tests/ref/fate/matroska-flac-extradata-update > @@ -1,5 +1,5 @@ > -83aca2772c52f6f802cac288f889382b *tests/data/fate/matroska-flac-extradata-update.matroska > -2019 tests/data/fate/matroska-flac-extradata-update.matroska > +5f6a67a45906f1bc7dd11d840470b0e4 *tests/data/fate/matroska-flac-extradata-update.matroska > +2071 tests/data/fate/matroska-flac-extradata-update.matroska > #extradata 0: 34, 0x7acb09e7 > #extradata 1: 34, 0x7acb09e7 > #extradata 2: 34, 0x443402dd > diff --git a/tests/ref/lavf/mka b/tests/ref/lavf/mka > index b3c4117d92..24ccef51fd 100644 > --- a/tests/ref/lavf/mka > +++ b/tests/ref/lavf/mka > @@ -1,3 +1,3 @@ > -0d48d93057f14704f6b839bb15e7328a *tests/data/lavf/lavf.mka > -43552 tests/data/lavf/lavf.mka > +df7155d4333e9993c9ea2a9d53868881 *tests/data/lavf/lavf.mka > +43580 tests/data/lavf/lavf.mka > tests/data/lavf/lavf.mka CRC=0x3a1da17e > -- > 2.20.1 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Steve Lhomme: >> On April 5, 2020 5:59 PM Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote: >> >> >> The Matroska muxer currently only adds CuePoints in three cases: >> a) For video keyframes. b) For the first audio frame in a new Cluster if >> in DASH-mode. c) For subtitles. This means that ordinary Matroska audio >> files won't have any Cues which impedes seeking. >> >> This commit changes this. For every track in a file without video track >> it is checked and tracked whether a Cue entry has already been added >> for said track for the current Cluster. This is used to add a Cue entry >> for each first packet of each track in each Cluster. >> >> Implements #3149. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> libavformat/matroskaenc.c | 21 +++++++++++-------- >> tests/ref/fate/aac-autobsf-adtstoasc | 4 ++-- >> tests/ref/fate/matroska-flac-extradata-update | 4 ++-- >> tests/ref/lavf/mka | 4 ++-- >> 4 files changed, 18 insertions(+), 15 deletions(-) >> >> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c >> index b9bfd34756..a469d48cc0 100644 >> --- a/libavformat/matroskaenc.c >> +++ b/libavformat/matroskaenc.c >> @@ -2120,6 +2120,10 @@ static void mkv_end_cluster(AVFormatContext *s) >> MatroskaMuxContext *mkv = s->priv_data; >> >> end_ebml_master_crc32(s->pb, &mkv->cluster_bc, mkv, MATROSKA_ID_CLUSTER, 0, 1); >> + if (!mkv->have_video) { > > You could initialize it even for video. In case in the future you want a Cue only on the first video keyframe of the Cluster. > Unless the first keyframe is extremely small (or more precisely: unless the Cluster contains less than 4KB when deciding whether to close and reopen the current Cluster), this muxer automatically opens a new cluster upon encountering a video keyframe, so I don't think that this need will arise. >> + for (unsigned i = 0; i < s->nb_streams; i++) >> + mkv->tracks[i].has_cue = 0; >> + } >> mkv->cluster_pos = -1; >> avio_write_marker(s->pb, AV_NOPTS_VALUE, AVIO_DATA_MARKER_FLUSH_POINT); >> } >> @@ -2222,7 +2226,7 @@ static int mkv_check_new_extra_data(AVFormatContext *s, AVPacket *pkt) >> return 0; >> } >> >> -static int mkv_write_packet_internal(AVFormatContext *s, AVPacket *pkt, int add_cue) >> +static int mkv_write_packet_internal(AVFormatContext *s, AVPacket *pkt) >> { >> MatroskaMuxContext *mkv = s->priv_data; >> AVIOContext *pb; >> @@ -2268,10 +2272,12 @@ static int mkv_write_packet_internal(AVFormatContext *s, AVPacket *pkt, int add_ >> ret = mkv_write_block(s, pb, MATROSKA_ID_SIMPLEBLOCK, pkt, keyframe); >> if (ret < 0) >> return ret; >> - if ((s->pb->seekable & AVIO_SEEKABLE_NORMAL) && (par->codec_type == AVMEDIA_TYPE_VIDEO && keyframe || add_cue)) { >> + if ((s->pb->seekable & AVIO_SEEKABLE_NORMAL) && keyframe && >> + (par->codec_type == AVMEDIA_TYPE_VIDEO || !mkv->have_video && !track->has_cue)) { > > You may restrict this to Audio/Subtitles. Other streams/tracks may not create a cue entry. > I'm not sure what you mean. Do you suggest that I change the last part to !mkv->have_video && !track->has_cue && (par->codec_type == AVMEDIA_TYPE_AUDIO || par->codec_type == AVMEDIA_TYPE_SUBTITLES)? This is unnecessary, as this muxer does not support anything else than audio, video and subtitles. Should this change, then some parts will need to be changed and this part may very well be among these parts, but right now applying this modification is pointless (and IMO confusing: every time I'd look at this code I would ask myself: "What is the point of this part of the check? It is automatically true."). >> ret = mkv_add_cuepoint(mkv, pkt->stream_index, ts, >> mkv->cluster_pos, relative_packet_pos, -1); >> if (ret < 0) return ret; >> + track->has_cue = 1; >> } >> } else { >> if (par->codec_id == AV_CODEC_ID_WEBVTT) { >> @@ -2336,8 +2342,7 @@ static int mkv_write_packet(AVFormatContext *s, AVPacket *pkt) >> // on seeing key frames. >> start_new_cluster = keyframe; >> } else if (mkv->is_dash && codec_type == AVMEDIA_TYPE_AUDIO && >> - (mkv->cluster_pos == -1 || >> - cluster_time > mkv->cluster_time_limit)) { >> + cluster_time > mkv->cluster_time_limit) { > > Is this related to this patch ? It currently means that if the Cluster did not write any Block then a new cluster is not needed. > The start_new_cluster variable is currently used for two things: To determine whether to end the current Cluster (in which case a new Cluster is automatically opened in mkv_write_packet_internal() -- the check used there amounts to "if there is no open Cluster right now, then open one") and also to know whether to add a Cue entry for DASH audio. As has been stated in the commit message, this was the one special case where Cues were written for audio files. The only reason the above check "mkv->cluster_pos == -1" exists is to make sure that the very first audio block of the very first Cluster also gets a Cue entry; this part is not used to determine whether to end the current Cluster, because there obviously is no current Cluster before the very first Cluster has been opened (and mkv_end_cluster() is of course never called if there is no open Cluster right now). But all the special code for adding Cues to DASH audio is now superfluous: DASH audio files have no video tracks, ergo the generic code for adding Cues now takes over this job. Which means that the above check can (and should) be remove. (This is also the reason why the signature of mkv_write_packet_internal() changed: There is no need to externally prescribe anymore whether a packet which would otherwise not get a Cue entry should get a Cue entry.) - Andreas >> // For DASH audio, we create a Cluster based on cluster_time_limit >> start_new_cluster = 1; >> } else if (!mkv->is_dash && >> @@ -2361,9 +2366,7 @@ static int mkv_write_packet(AVFormatContext *s, AVPacket *pkt) >> >> // check if we have an audio packet cached >> if (mkv->cur_audio_pkt.size > 0) { >> - // for DASH audio, a CuePoint has to be added when there is a new cluster. >> - ret = mkv_write_packet_internal(s, &mkv->cur_audio_pkt, >> - mkv->is_dash ? start_new_cluster : 0); >> + ret = mkv_write_packet_internal(s, &mkv->cur_audio_pkt); >> av_packet_unref(&mkv->cur_audio_pkt); >> if (ret < 0) { >> av_log(s, AV_LOG_ERROR, >> @@ -2378,7 +2381,7 @@ static int mkv_write_packet(AVFormatContext *s, AVPacket *pkt) >> if (pkt->size > 0) >> ret = av_packet_ref(&mkv->cur_audio_pkt, pkt); >> } else >> - ret = mkv_write_packet_internal(s, pkt, 0); >> + ret = mkv_write_packet_internal(s, pkt); >> return ret; >> } >> >> @@ -2406,7 +2409,7 @@ static int mkv_write_trailer(AVFormatContext *s) >> >> // check if we have an audio packet cached >> if (mkv->cur_audio_pkt.size > 0) { >> - ret = mkv_write_packet_internal(s, &mkv->cur_audio_pkt, 0); >> + ret = mkv_write_packet_internal(s, &mkv->cur_audio_pkt); >> if (ret < 0) { >> av_log(s, AV_LOG_ERROR, >> "Could not write cached audio packet ret:%d\n", ret); >> diff --git a/tests/ref/fate/aac-autobsf-adtstoasc b/tests/ref/fate/aac-autobsf-adtstoasc >> index f1c6f889d4..d9191fb37f 100644 >> --- a/tests/ref/fate/aac-autobsf-adtstoasc >> +++ b/tests/ref/fate/aac-autobsf-adtstoasc >> @@ -1,5 +1,5 @@ >> -9d0c81ce285a84c0137316004d091d95 *tests/data/fate/aac-autobsf-adtstoasc.matroska >> -6620 tests/data/fate/aac-autobsf-adtstoasc.matroska >> +76a14cc1b3292c7f724006d56b7e2eac *tests/data/fate/aac-autobsf-adtstoasc.matroska >> +6648 tests/data/fate/aac-autobsf-adtstoasc.matroska >> #extradata 0: 2, 0x0030001c >> #tb 0: 1/1000 >> #media_type 0: audio >> diff --git a/tests/ref/fate/matroska-flac-extradata-update b/tests/ref/fate/matroska-flac-extradata-update >> index dfb2851b0f..16b268c4a8 100644 >> --- a/tests/ref/fate/matroska-flac-extradata-update >> +++ b/tests/ref/fate/matroska-flac-extradata-update >> @@ -1,5 +1,5 @@ >> -83aca2772c52f6f802cac288f889382b *tests/data/fate/matroska-flac-extradata-update.matroska >> -2019 tests/data/fate/matroska-flac-extradata-update.matroska >> +5f6a67a45906f1bc7dd11d840470b0e4 *tests/data/fate/matroska-flac-extradata-update.matroska >> +2071 tests/data/fate/matroska-flac-extradata-update.matroska >> #extradata 0: 34, 0x7acb09e7 >> #extradata 1: 34, 0x7acb09e7 >> #extradata 2: 34, 0x443402dd >> diff --git a/tests/ref/lavf/mka b/tests/ref/lavf/mka >> index b3c4117d92..24ccef51fd 100644 >> --- a/tests/ref/lavf/mka >> +++ b/tests/ref/lavf/mka >> @@ -1,3 +1,3 @@ >> -0d48d93057f14704f6b839bb15e7328a *tests/data/lavf/lavf.mka >> -43552 tests/data/lavf/lavf.mka >> +df7155d4333e9993c9ea2a9d53868881 *tests/data/lavf/lavf.mka >> +43580 tests/data/lavf/lavf.mka >> tests/data/lavf/lavf.mka CRC=0x3a1da17e >> -- >> 2.20.1 >> >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> To unsubscribe, visit link above, or email >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
Andreas Rheinhardt: > The Matroska muxer currently only adds CuePoints in three cases: > a) For video keyframes. b) For the first audio frame in a new Cluster if > in DASH-mode. c) For subtitles. This means that ordinary Matroska audio > files won't have any Cues which impedes seeking. > > This commit changes this. For every track in a file without video track > it is checked and tracked whether a Cue entry has already been added > for said track for the current Cluster. This is used to add a Cue entry > for each first packet of each track in each Cluster. > > Implements #3149. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavformat/matroskaenc.c | 21 +++++++++++-------- > tests/ref/fate/aac-autobsf-adtstoasc | 4 ++-- > tests/ref/fate/matroska-flac-extradata-update | 4 ++-- > tests/ref/lavf/mka | 4 ++-- > 4 files changed, 18 insertions(+), 15 deletions(-) > > diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c > index b9bfd34756..a469d48cc0 100644 > --- a/libavformat/matroskaenc.c > +++ b/libavformat/matroskaenc.c > @@ -2120,6 +2120,10 @@ static void mkv_end_cluster(AVFormatContext *s) > MatroskaMuxContext *mkv = s->priv_data; > > end_ebml_master_crc32(s->pb, &mkv->cluster_bc, mkv, MATROSKA_ID_CLUSTER, 0, 1); > + if (!mkv->have_video) { > + for (unsigned i = 0; i < s->nb_streams; i++) > + mkv->tracks[i].has_cue = 0; > + } > mkv->cluster_pos = -1; > avio_write_marker(s->pb, AV_NOPTS_VALUE, AVIO_DATA_MARKER_FLUSH_POINT); > } > @@ -2222,7 +2226,7 @@ static int mkv_check_new_extra_data(AVFormatContext *s, AVPacket *pkt) > return 0; > } > > -static int mkv_write_packet_internal(AVFormatContext *s, AVPacket *pkt, int add_cue) > +static int mkv_write_packet_internal(AVFormatContext *s, AVPacket *pkt) > { > MatroskaMuxContext *mkv = s->priv_data; > AVIOContext *pb; > @@ -2268,10 +2272,12 @@ static int mkv_write_packet_internal(AVFormatContext *s, AVPacket *pkt, int add_ > ret = mkv_write_block(s, pb, MATROSKA_ID_SIMPLEBLOCK, pkt, keyframe); > if (ret < 0) > return ret; > - if ((s->pb->seekable & AVIO_SEEKABLE_NORMAL) && (par->codec_type == AVMEDIA_TYPE_VIDEO && keyframe || add_cue)) { > + if ((s->pb->seekable & AVIO_SEEKABLE_NORMAL) && keyframe && > + (par->codec_type == AVMEDIA_TYPE_VIDEO || !mkv->have_video && !track->has_cue)) { > ret = mkv_add_cuepoint(mkv, pkt->stream_index, ts, > mkv->cluster_pos, relative_packet_pos, -1); > if (ret < 0) return ret; > + track->has_cue = 1; > } > } else { > if (par->codec_id == AV_CODEC_ID_WEBVTT) { > @@ -2336,8 +2342,7 @@ static int mkv_write_packet(AVFormatContext *s, AVPacket *pkt) > // on seeing key frames. > start_new_cluster = keyframe; > } else if (mkv->is_dash && codec_type == AVMEDIA_TYPE_AUDIO && > - (mkv->cluster_pos == -1 || > - cluster_time > mkv->cluster_time_limit)) { > + cluster_time > mkv->cluster_time_limit) { > // For DASH audio, we create a Cluster based on cluster_time_limit > start_new_cluster = 1; > } else if (!mkv->is_dash && > @@ -2361,9 +2366,7 @@ static int mkv_write_packet(AVFormatContext *s, AVPacket *pkt) > > // check if we have an audio packet cached > if (mkv->cur_audio_pkt.size > 0) { > - // for DASH audio, a CuePoint has to be added when there is a new cluster. > - ret = mkv_write_packet_internal(s, &mkv->cur_audio_pkt, > - mkv->is_dash ? start_new_cluster : 0); > + ret = mkv_write_packet_internal(s, &mkv->cur_audio_pkt); > av_packet_unref(&mkv->cur_audio_pkt); > if (ret < 0) { > av_log(s, AV_LOG_ERROR, > @@ -2378,7 +2381,7 @@ static int mkv_write_packet(AVFormatContext *s, AVPacket *pkt) > if (pkt->size > 0) > ret = av_packet_ref(&mkv->cur_audio_pkt, pkt); > } else > - ret = mkv_write_packet_internal(s, pkt, 0); > + ret = mkv_write_packet_internal(s, pkt); > return ret; > } > > @@ -2406,7 +2409,7 @@ static int mkv_write_trailer(AVFormatContext *s) > > // check if we have an audio packet cached > if (mkv->cur_audio_pkt.size > 0) { > - ret = mkv_write_packet_internal(s, &mkv->cur_audio_pkt, 0); > + ret = mkv_write_packet_internal(s, &mkv->cur_audio_pkt); > if (ret < 0) { > av_log(s, AV_LOG_ERROR, > "Could not write cached audio packet ret:%d\n", ret); > diff --git a/tests/ref/fate/aac-autobsf-adtstoasc b/tests/ref/fate/aac-autobsf-adtstoasc > index f1c6f889d4..d9191fb37f 100644 > --- a/tests/ref/fate/aac-autobsf-adtstoasc > +++ b/tests/ref/fate/aac-autobsf-adtstoasc > @@ -1,5 +1,5 @@ > -9d0c81ce285a84c0137316004d091d95 *tests/data/fate/aac-autobsf-adtstoasc.matroska > -6620 tests/data/fate/aac-autobsf-adtstoasc.matroska > +76a14cc1b3292c7f724006d56b7e2eac *tests/data/fate/aac-autobsf-adtstoasc.matroska > +6648 tests/data/fate/aac-autobsf-adtstoasc.matroska > #extradata 0: 2, 0x0030001c > #tb 0: 1/1000 > #media_type 0: audio > diff --git a/tests/ref/fate/matroska-flac-extradata-update b/tests/ref/fate/matroska-flac-extradata-update > index dfb2851b0f..16b268c4a8 100644 > --- a/tests/ref/fate/matroska-flac-extradata-update > +++ b/tests/ref/fate/matroska-flac-extradata-update > @@ -1,5 +1,5 @@ > -83aca2772c52f6f802cac288f889382b *tests/data/fate/matroska-flac-extradata-update.matroska > -2019 tests/data/fate/matroska-flac-extradata-update.matroska > +5f6a67a45906f1bc7dd11d840470b0e4 *tests/data/fate/matroska-flac-extradata-update.matroska > +2071 tests/data/fate/matroska-flac-extradata-update.matroska > #extradata 0: 34, 0x7acb09e7 > #extradata 1: 34, 0x7acb09e7 > #extradata 2: 34, 0x443402dd > diff --git a/tests/ref/lavf/mka b/tests/ref/lavf/mka > index b3c4117d92..24ccef51fd 100644 > --- a/tests/ref/lavf/mka > +++ b/tests/ref/lavf/mka > @@ -1,3 +1,3 @@ > -0d48d93057f14704f6b839bb15e7328a *tests/data/lavf/lavf.mka > -43552 tests/data/lavf/lavf.mka > +df7155d4333e9993c9ea2a9d53868881 *tests/data/lavf/lavf.mka > +43580 tests/data/lavf/lavf.mka > tests/data/lavf/lavf.mka CRC=0x3a1da17e > Given that this here was now on the ML for over two weeks and given that Steve just confirmed to me via IRC that all outstanding Matroska muxer patches are LGTM for him, I will merge them tomorrow if there are no objections. This means that if someone has better naming suggestions for the FlagDefault option [1] he should say so now so that they can be considered. - Andreas [1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-April/259802.html
diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c index b9bfd34756..a469d48cc0 100644 --- a/libavformat/matroskaenc.c +++ b/libavformat/matroskaenc.c @@ -2120,6 +2120,10 @@ static void mkv_end_cluster(AVFormatContext *s) MatroskaMuxContext *mkv = s->priv_data; end_ebml_master_crc32(s->pb, &mkv->cluster_bc, mkv, MATROSKA_ID_CLUSTER, 0, 1); + if (!mkv->have_video) { + for (unsigned i = 0; i < s->nb_streams; i++) + mkv->tracks[i].has_cue = 0; + } mkv->cluster_pos = -1; avio_write_marker(s->pb, AV_NOPTS_VALUE, AVIO_DATA_MARKER_FLUSH_POINT); } @@ -2222,7 +2226,7 @@ static int mkv_check_new_extra_data(AVFormatContext *s, AVPacket *pkt) return 0; } -static int mkv_write_packet_internal(AVFormatContext *s, AVPacket *pkt, int add_cue) +static int mkv_write_packet_internal(AVFormatContext *s, AVPacket *pkt) { MatroskaMuxContext *mkv = s->priv_data; AVIOContext *pb; @@ -2268,10 +2272,12 @@ static int mkv_write_packet_internal(AVFormatContext *s, AVPacket *pkt, int add_ ret = mkv_write_block(s, pb, MATROSKA_ID_SIMPLEBLOCK, pkt, keyframe); if (ret < 0) return ret; - if ((s->pb->seekable & AVIO_SEEKABLE_NORMAL) && (par->codec_type == AVMEDIA_TYPE_VIDEO && keyframe || add_cue)) { + if ((s->pb->seekable & AVIO_SEEKABLE_NORMAL) && keyframe && + (par->codec_type == AVMEDIA_TYPE_VIDEO || !mkv->have_video && !track->has_cue)) { ret = mkv_add_cuepoint(mkv, pkt->stream_index, ts, mkv->cluster_pos, relative_packet_pos, -1); if (ret < 0) return ret; + track->has_cue = 1; } } else { if (par->codec_id == AV_CODEC_ID_WEBVTT) { @@ -2336,8 +2342,7 @@ static int mkv_write_packet(AVFormatContext *s, AVPacket *pkt) // on seeing key frames. start_new_cluster = keyframe; } else if (mkv->is_dash && codec_type == AVMEDIA_TYPE_AUDIO && - (mkv->cluster_pos == -1 || - cluster_time > mkv->cluster_time_limit)) { + cluster_time > mkv->cluster_time_limit) { // For DASH audio, we create a Cluster based on cluster_time_limit start_new_cluster = 1; } else if (!mkv->is_dash && @@ -2361,9 +2366,7 @@ static int mkv_write_packet(AVFormatContext *s, AVPacket *pkt) // check if we have an audio packet cached if (mkv->cur_audio_pkt.size > 0) { - // for DASH audio, a CuePoint has to be added when there is a new cluster. - ret = mkv_write_packet_internal(s, &mkv->cur_audio_pkt, - mkv->is_dash ? start_new_cluster : 0); + ret = mkv_write_packet_internal(s, &mkv->cur_audio_pkt); av_packet_unref(&mkv->cur_audio_pkt); if (ret < 0) { av_log(s, AV_LOG_ERROR, @@ -2378,7 +2381,7 @@ static int mkv_write_packet(AVFormatContext *s, AVPacket *pkt) if (pkt->size > 0) ret = av_packet_ref(&mkv->cur_audio_pkt, pkt); } else - ret = mkv_write_packet_internal(s, pkt, 0); + ret = mkv_write_packet_internal(s, pkt); return ret; } @@ -2406,7 +2409,7 @@ static int mkv_write_trailer(AVFormatContext *s) // check if we have an audio packet cached if (mkv->cur_audio_pkt.size > 0) { - ret = mkv_write_packet_internal(s, &mkv->cur_audio_pkt, 0); + ret = mkv_write_packet_internal(s, &mkv->cur_audio_pkt); if (ret < 0) { av_log(s, AV_LOG_ERROR, "Could not write cached audio packet ret:%d\n", ret); diff --git a/tests/ref/fate/aac-autobsf-adtstoasc b/tests/ref/fate/aac-autobsf-adtstoasc index f1c6f889d4..d9191fb37f 100644 --- a/tests/ref/fate/aac-autobsf-adtstoasc +++ b/tests/ref/fate/aac-autobsf-adtstoasc @@ -1,5 +1,5 @@ -9d0c81ce285a84c0137316004d091d95 *tests/data/fate/aac-autobsf-adtstoasc.matroska -6620 tests/data/fate/aac-autobsf-adtstoasc.matroska +76a14cc1b3292c7f724006d56b7e2eac *tests/data/fate/aac-autobsf-adtstoasc.matroska +6648 tests/data/fate/aac-autobsf-adtstoasc.matroska #extradata 0: 2, 0x0030001c #tb 0: 1/1000 #media_type 0: audio diff --git a/tests/ref/fate/matroska-flac-extradata-update b/tests/ref/fate/matroska-flac-extradata-update index dfb2851b0f..16b268c4a8 100644 --- a/tests/ref/fate/matroska-flac-extradata-update +++ b/tests/ref/fate/matroska-flac-extradata-update @@ -1,5 +1,5 @@ -83aca2772c52f6f802cac288f889382b *tests/data/fate/matroska-flac-extradata-update.matroska -2019 tests/data/fate/matroska-flac-extradata-update.matroska +5f6a67a45906f1bc7dd11d840470b0e4 *tests/data/fate/matroska-flac-extradata-update.matroska +2071 tests/data/fate/matroska-flac-extradata-update.matroska #extradata 0: 34, 0x7acb09e7 #extradata 1: 34, 0x7acb09e7 #extradata 2: 34, 0x443402dd diff --git a/tests/ref/lavf/mka b/tests/ref/lavf/mka index b3c4117d92..24ccef51fd 100644 --- a/tests/ref/lavf/mka +++ b/tests/ref/lavf/mka @@ -1,3 +1,3 @@ -0d48d93057f14704f6b839bb15e7328a *tests/data/lavf/lavf.mka -43552 tests/data/lavf/lavf.mka +df7155d4333e9993c9ea2a9d53868881 *tests/data/lavf/lavf.mka +43580 tests/data/lavf/lavf.mka tests/data/lavf/lavf.mka CRC=0x3a1da17e
The Matroska muxer currently only adds CuePoints in three cases: a) For video keyframes. b) For the first audio frame in a new Cluster if in DASH-mode. c) For subtitles. This means that ordinary Matroska audio files won't have any Cues which impedes seeking. This commit changes this. For every track in a file without video track it is checked and tracked whether a Cue entry has already been added for said track for the current Cluster. This is used to add a Cue entry for each first packet of each track in each Cluster. Implements #3149. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavformat/matroskaenc.c | 21 +++++++++++-------- tests/ref/fate/aac-autobsf-adtstoasc | 4 ++-- tests/ref/fate/matroska-flac-extradata-update | 4 ++-- tests/ref/lavf/mka | 4 ++-- 4 files changed, 18 insertions(+), 15 deletions(-)