[FFmpeg-devel] [PATCH 12/20] avformat/matroskaenc: Improve Cues in case of no video
Andreas Rheinhardt
andreas.rheinhardt at gmail.com
Sun Apr 19 11:15:17 EEST 2020
Steve Lhomme:
>> On April 5, 2020 5:59 PM Andreas Rheinhardt <andreas.rheinhardt at 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 at 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 at ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>
More information about the ffmpeg-devel
mailing list