From 03d31ef39c18990a6fbdfad99c69084b60af7758 Mon Sep 17 00:00:00 2001 From: Andreas Rheinhardt Date: Sun, 29 Dec 2019 03:56:34 +0100 Subject: [PATCH] avformat/matroskaenc: Remove special code for writing subtitles Once upon a time, mkv_write_block() only wrote a (Simple)Block, not a BlockGroup which is needed for subtitles to convey the duration. But with the introduction of support for writing BlockAdditions and DiscardPadding (both of which require a BlockGroup), mkv_write_block() can also open and close a BlockGroup of its own. This naturally led to some code duplication which is removed in this commit. This new code leads to one regression: It always uses eight bytes for the BlockGroup's length field, whereas the earlier code usually used the lowest amount of bytes needed. This will be fixed in a future commit. This temporary regression is also the reason for changes to the binsub-mksenc and matroska-zero-length-block fate tests. Signed-off-by: Andreas Rheinhardt --- libavformat/matroskaenc.c | 42 ++++++++++------------- tests/ref/fate/binsub-mksenc | 2 +- tests/ref/fate/matroska-zero-length-block | 4 +-- 3 files changed, 21 insertions(+), 27 deletions(-) diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c index 4d30ebae71..2ae033bce1 100644 --- a/libavformat/matroskaenc.c +++ b/libavformat/matroskaenc.c @@ -2418,7 +2418,7 @@ static int mkv_reformat_av1(MatroskaMuxContext *mkv, AVIOContext *pb, } static int mkv_write_block(AVFormatContext *s, AVIOContext *pb, - uint32_t blockid, const AVPacket *pkt, int keyframe) + const AVPacket *pkt, int keyframe, uint64_t duration) { MatroskaMuxContext *mkv = s->priv_data; AVCodecParameters *par = s->streams[pkt->stream_index]->codecpar; @@ -2428,10 +2428,10 @@ static int mkv_write_block(AVFormatContext *s, AVIOContext *pb, int err = 0, offset = 0, size = pkt->size; int64_t ts = track->write_dts ? pkt->dts : pkt->pts; uint64_t additional_id; + uint32_t blockid = MATROSKA_ID_SIMPLEBLOCK; int64_t discard_padding = 0; unsigned track_number = track->track_num; ebml_master block_group, block_additions, block_more; - int blockgroup_already_opened = blockid == MATROSKA_ID_BLOCK; ts += track->ts_offset; @@ -2466,7 +2466,7 @@ static int mkv_write_block(AVFormatContext *s, AVIOContext *pb, side_data = av_packet_get_side_data(pkt, AV_PKT_DATA_SKIP_SAMPLES, &side_data_size); - if (side_data && side_data_size >= 10 && !blockgroup_already_opened) { + if (side_data && side_data_size >= 10) { discard_padding = av_rescale_q(AV_RL32(side_data + 4), (AVRational){1, par->sample_rate}, (AVRational){1, 1000000000}); @@ -2477,8 +2477,7 @@ static int mkv_write_block(AVFormatContext *s, AVIOContext *pb, &side_data_size); if (side_data) { // Only the Codec-specific BlockMore (id == 1) is currently supported. - if (side_data_size < 8 || (additional_id = AV_RB64(side_data)) != 1 || - blockgroup_already_opened) { + if (side_data_size < 8 || (additional_id = AV_RB64(side_data)) != 1) { side_data_size = 0; } else { side_data += 8; @@ -2486,7 +2485,7 @@ static int mkv_write_block(AVFormatContext *s, AVIOContext *pb, } } - if (side_data_size || discard_padding) { + if (side_data_size || discard_padding || duration) { block_group = start_ebml_master(pb, MATROSKA_ID_BLOCKGROUP, 0); blockid = MATROSKA_ID_BLOCK; } @@ -2502,6 +2501,9 @@ static int mkv_write_block(AVFormatContext *s, AVIOContext *pb, avio_write(pb, data + offset, size); } + if (duration) + put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, duration); + if (blockid == MATROSKA_ID_BLOCK && !keyframe) put_ebml_sint(pb, MATROSKA_ID_BLOCKREFERENCE, track->last_timestamp - ts); track->last_timestamp = ts; @@ -2520,7 +2522,7 @@ static int mkv_write_block(AVFormatContext *s, AVIOContext *pb, end_ebml_master(pb, block_more); end_ebml_master(pb, block_additions); } - if (side_data_size || discard_padding) + if (blockid == MATROSKA_ID_BLOCK) end_ebml_master(pb, block_group); return 0; @@ -2697,8 +2699,11 @@ static int mkv_write_packet_internal(AVFormatContext *s, const AVPacket *pkt) AVIOContext *pb; AVCodecParameters *par = s->streams[pkt->stream_index]->codecpar; mkv_track *track = &mkv->tracks[pkt->stream_index]; - int keyframe = !!(pkt->flags & AV_PKT_FLAG_KEY); - int64_t duration = pkt->duration; + int is_sub = par->codec_type == AVMEDIA_TYPE_SUBTITLE; + /* All subtitle blocks are considered to be keyframes. */ + int keyframe = is_sub || !!(pkt->flags & AV_PKT_FLAG_KEY); + int64_t duration = FFMAX(pkt->duration, 0); + int64_t write_duration = is_sub ? duration : 0; int ret; int64_t ts = track->write_dts ? pkt->dts : pkt->pts; int64_t relative_packet_pos; @@ -2735,9 +2740,8 @@ static int mkv_write_packet_internal(AVFormatContext *s, const AVPacket *pkt) relative_packet_pos = avio_tell(pb); - if (par->codec_type != AVMEDIA_TYPE_SUBTITLE || - (par->codec_id != AV_CODEC_ID_WEBVTT && duration <= 0)) { - ret = mkv_write_block(s, pb, MATROSKA_ID_SIMPLEBLOCK, pkt, keyframe); + if (par->codec_id != AV_CODEC_ID_WEBVTT) { + ret = mkv_write_block(s, pb, pkt, keyframe, write_duration); if (ret < 0) return ret; if (keyframe && IS_SEEKABLE(s->pb, mkv) && @@ -2745,26 +2749,16 @@ static int mkv_write_packet_internal(AVFormatContext *s, const AVPacket *pkt) par->codec_type == AVMEDIA_TYPE_SUBTITLE || !mkv->have_video && !track->has_cue)) { ret = mkv_add_cuepoint(mkv, pkt->stream_index, ts, - mkv->cluster_pos, relative_packet_pos, 0); + mkv->cluster_pos, relative_packet_pos, + write_duration); if (ret < 0) return ret; track->has_cue = 1; } } else { - if (par->codec_id == AV_CODEC_ID_WEBVTT) { ret = mkv_write_vtt_blocks(s, pb, pkt); if (ret < 0) return ret; - } else { - ebml_master blockgroup = start_ebml_master(pb, MATROSKA_ID_BLOCKGROUP, - mkv_blockgroup_size(pkt->size, - track->track_num_size)); - - /* All subtitle blocks are considered to be keyframes. */ - mkv_write_block(s, pb, MATROSKA_ID_BLOCK, pkt, 1); - put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, duration); - end_ebml_master(pb, blockgroup); - } if (IS_SEEKABLE(s->pb, mkv)) { ret = mkv_add_cuepoint(mkv, pkt->stream_index, ts, diff --git a/tests/ref/fate/binsub-mksenc b/tests/ref/fate/binsub-mksenc index 184661a3f0..01e7db64f2 100644 --- a/tests/ref/fate/binsub-mksenc +++ b/tests/ref/fate/binsub-mksenc @@ -1 +1 @@ -490b1b4beb4a614c06004609039ce779 +9dd2ff92a3da9fb50405db3d05e41042 diff --git a/tests/ref/fate/matroska-zero-length-block b/tests/ref/fate/matroska-zero-length-block index 924cec1e3f..edee36a4ab 100644 --- a/tests/ref/fate/matroska-zero-length-block +++ b/tests/ref/fate/matroska-zero-length-block @@ -1,5 +1,5 @@ -f37ba7e8a30eaa33c1fd0ef77447fb41 *tests/data/fate/matroska-zero-length-block.matroska -636 tests/data/fate/matroska-zero-length-block.matroska +c09d3b89ed0795817d671deb041fca1b *tests/data/fate/matroska-zero-length-block.matroska +650 tests/data/fate/matroska-zero-length-block.matroska #tb 0: 1/1000 #media_type 0: subtitle #codec_id 0: subrip