diff mbox series

[FFmpeg-devel,12/20] avformat/matroskaenc: Improve Cues in case of no video

Message ID 20200405155928.9323-13-andreas.rheinhardt@gmail.com
State Accepted
Commit 945b92873061c66b82df892887cd9baf8146857b
Headers show
Series Matroska muxer patches
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Andreas Rheinhardt April 5, 2020, 3:59 p.m. UTC
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(-)

Comments

Jan Chren (rindeal) April 6, 2020, 3:31 a.m. UTC | #1
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
Andreas Rheinhardt April 6, 2020, 1:08 p.m. UTC | #2
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
Steve Lhomme April 19, 2020, 7:43 a.m. UTC | #3
> 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".
Andreas Rheinhardt April 19, 2020, 8:15 a.m. UTC | #4
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 April 20, 2020, 7:20 a.m. UTC | #5
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 mbox series

Patch

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