| Message ID | 20200618004743.95896-4-rcombs@rcombs.me |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/5] lavf: matroska subtitle muxer | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
rcombs: > --- > libavformat/matroskaenc.c | 30 ++++++++++++++++++++++++------ > 1 file changed, 24 insertions(+), 6 deletions(-) > > diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c > index 63d6b50e6a..94e6cc542a 100644 > --- a/libavformat/matroskaenc.c > +++ b/libavformat/matroskaenc.c > @@ -2117,22 +2117,25 @@ static int mkv_write_vtt_blocks(AVFormatContext *s, AVIOContext *pb, const AVPac > MatroskaMuxContext *mkv = s->priv_data; > mkv_track *track = &mkv->tracks[pkt->stream_index]; > ebml_master blockgroup; > - int id_size, settings_size, size; > - const char *id, *settings; > + int id_size = 0, settings_size = 0, comment_size = 0, size = pkt->size; > + const char *id, *settings, *comment; > int64_t ts = track->write_dts ? pkt->dts : pkt->pts; > const int flags = 0; > > - id_size = 0; > id = av_packet_get_side_data(pkt, AV_PKT_DATA_WEBVTT_IDENTIFIER, > &id_size); > id = id ? id : ""; > > - settings_size = 0; > settings = av_packet_get_side_data(pkt, AV_PKT_DATA_WEBVTT_SETTINGS, > &settings_size); > settings = settings ? settings : ""; > > - size = id_size + 1 + settings_size + 1 + pkt->size; > + comment = av_packet_get_side_data(pkt, AV_PKT_DATA_WEBVTT_COMMENT, > + &comment_size); > + comment = comment ? comment : ""; > + > + if (mkv->mode == MODE_WEBM) > + size += id_size + 1 + settings_size + 1; > > /* The following string is identical to the one in mkv_write_block so that > * only one copy needs to exist in binaries. */ > @@ -2151,7 +2154,22 @@ static int mkv_write_vtt_blocks(AVFormatContext *s, AVIOContext *pb, const AVPac > put_ebml_num(pb, track->track_num, track->track_num_size); > avio_wb16(pb, ts - mkv->cluster_pts); > avio_w8(pb, flags); > - avio_printf(pb, "%.*s\n%.*s\n%.*s", id_size, id, settings_size, settings, pkt->size, pkt->data); > + if (mkv->mode == MODE_WEBM) > + avio_printf(pb, "%.*s\n%.*s\n", id_size, id, settings_size, settings); > + avio_write(pb, pkt->data, pkt->size); > + > + if (mkv->mode != MODE_WEBM && (id_size || settings_size || comment_size)) { > + ebml_master block_additions = start_ebml_master(pb, MATROSKA_ID_BLOCKADDITIONS, 0); > + ebml_master block_more = start_ebml_master(pb, MATROSKA_ID_BLOCKMORE, 0); > + /* Until dbc50f8a our demuxer used a wrong default value > + * of BlockAddID, so we write it unconditionally. */ > + put_ebml_uint (pb, MATROSKA_ID_BLOCKADDID, 1); > + put_ebml_id(pb, MATROSKA_ID_BLOCKADDITIONAL); > + put_ebml_length(pb, id_size + 1 + settings_size + 1 + comment_size, 0); > + avio_printf(pb, "%.*s\n%.*s\n%.*s", id_size, id, settings_size, settings, comment_size, comment); > + end_ebml_master(pb, block_more); > + end_ebml_master(pb, block_additions); > + } > > put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, pkt->duration); > end_ebml_master(pb, blockgroup); Did you test this? Did it work? It shouldn't. The initial guess for the size of blockgroup is based solely on pkt->size (and TrackNumber); it does not take the size of the side data into account and therefore may underestimate the amount of bytes needed for the blockgroup length field. And this leads to an assert. - Andreas
> On Jun 20, 2020, at 04:12, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote: > > rcombs: >> --- >> libavformat/matroskaenc.c | 30 ++++++++++++++++++++++++------ >> 1 file changed, 24 insertions(+), 6 deletions(-) >> >> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c >> index 63d6b50e6a..94e6cc542a 100644 >> --- a/libavformat/matroskaenc.c >> +++ b/libavformat/matroskaenc.c >> @@ -2117,22 +2117,25 @@ static int mkv_write_vtt_blocks(AVFormatContext *s, AVIOContext *pb, const AVPac >> MatroskaMuxContext *mkv = s->priv_data; >> mkv_track *track = &mkv->tracks[pkt->stream_index]; >> ebml_master blockgroup; >> - int id_size, settings_size, size; >> - const char *id, *settings; >> + int id_size = 0, settings_size = 0, comment_size = 0, size = pkt->size; >> + const char *id, *settings, *comment; >> int64_t ts = track->write_dts ? pkt->dts : pkt->pts; >> const int flags = 0; >> >> - id_size = 0; >> id = av_packet_get_side_data(pkt, AV_PKT_DATA_WEBVTT_IDENTIFIER, >> &id_size); >> id = id ? id : ""; >> >> - settings_size = 0; >> settings = av_packet_get_side_data(pkt, AV_PKT_DATA_WEBVTT_SETTINGS, >> &settings_size); >> settings = settings ? settings : ""; >> >> - size = id_size + 1 + settings_size + 1 + pkt->size; >> + comment = av_packet_get_side_data(pkt, AV_PKT_DATA_WEBVTT_COMMENT, >> + &comment_size); >> + comment = comment ? comment : ""; >> + >> + if (mkv->mode == MODE_WEBM) >> + size += id_size + 1 + settings_size + 1; >> >> /* The following string is identical to the one in mkv_write_block so that >> * only one copy needs to exist in binaries. */ >> @@ -2151,7 +2154,22 @@ static int mkv_write_vtt_blocks(AVFormatContext *s, AVIOContext *pb, const AVPac >> put_ebml_num(pb, track->track_num, track->track_num_size); >> avio_wb16(pb, ts - mkv->cluster_pts); >> avio_w8(pb, flags); >> - avio_printf(pb, "%.*s\n%.*s\n%.*s", id_size, id, settings_size, settings, pkt->size, pkt->data); >> + if (mkv->mode == MODE_WEBM) >> + avio_printf(pb, "%.*s\n%.*s\n", id_size, id, settings_size, settings); >> + avio_write(pb, pkt->data, pkt->size); >> + >> + if (mkv->mode != MODE_WEBM && (id_size || settings_size || comment_size)) { >> + ebml_master block_additions = start_ebml_master(pb, MATROSKA_ID_BLOCKADDITIONS, 0); >> + ebml_master block_more = start_ebml_master(pb, MATROSKA_ID_BLOCKMORE, 0); >> + /* Until dbc50f8a our demuxer used a wrong default value >> + * of BlockAddID, so we write it unconditionally. */ >> + put_ebml_uint (pb, MATROSKA_ID_BLOCKADDID, 1); >> + put_ebml_id(pb, MATROSKA_ID_BLOCKADDITIONAL); >> + put_ebml_length(pb, id_size + 1 + settings_size + 1 + comment_size, 0); >> + avio_printf(pb, "%.*s\n%.*s\n%.*s", id_size, id, settings_size, settings, comment_size, comment); >> + end_ebml_master(pb, block_more); >> + end_ebml_master(pb, block_additions); >> + } >> >> put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, pkt->duration); >> end_ebml_master(pb, blockgroup); > > Did you test this? Did it work? It shouldn't. The initial guess for the > size of blockgroup is based solely on pkt->size (and TrackNumber); it > does not take the size of the side data into account and therefore may > underestimate the amount of bytes needed for the blockgroup length > field. And this leads to an assert. I tested and it did work, but I see where the problem you mentioned comes from. Just, it'd only cause an assert failure (or observable output issue) if the BlockAdditions was large enough (and the size close enough to a byte boundary) that it would've made the length take up an additional byte, which didn't come up in my tests. Will fix, thanks. > > - Andreas > _______________________________________________ > 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".
diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c index 63d6b50e6a..94e6cc542a 100644 --- a/libavformat/matroskaenc.c +++ b/libavformat/matroskaenc.c @@ -2117,22 +2117,25 @@ static int mkv_write_vtt_blocks(AVFormatContext *s, AVIOContext *pb, const AVPac MatroskaMuxContext *mkv = s->priv_data; mkv_track *track = &mkv->tracks[pkt->stream_index]; ebml_master blockgroup; - int id_size, settings_size, size; - const char *id, *settings; + int id_size = 0, settings_size = 0, comment_size = 0, size = pkt->size; + const char *id, *settings, *comment; int64_t ts = track->write_dts ? pkt->dts : pkt->pts; const int flags = 0; - id_size = 0; id = av_packet_get_side_data(pkt, AV_PKT_DATA_WEBVTT_IDENTIFIER, &id_size); id = id ? id : ""; - settings_size = 0; settings = av_packet_get_side_data(pkt, AV_PKT_DATA_WEBVTT_SETTINGS, &settings_size); settings = settings ? settings : ""; - size = id_size + 1 + settings_size + 1 + pkt->size; + comment = av_packet_get_side_data(pkt, AV_PKT_DATA_WEBVTT_COMMENT, + &comment_size); + comment = comment ? comment : ""; + + if (mkv->mode == MODE_WEBM) + size += id_size + 1 + settings_size + 1; /* The following string is identical to the one in mkv_write_block so that * only one copy needs to exist in binaries. */ @@ -2151,7 +2154,22 @@ static int mkv_write_vtt_blocks(AVFormatContext *s, AVIOContext *pb, const AVPac put_ebml_num(pb, track->track_num, track->track_num_size); avio_wb16(pb, ts - mkv->cluster_pts); avio_w8(pb, flags); - avio_printf(pb, "%.*s\n%.*s\n%.*s", id_size, id, settings_size, settings, pkt->size, pkt->data); + if (mkv->mode == MODE_WEBM) + avio_printf(pb, "%.*s\n%.*s\n", id_size, id, settings_size, settings); + avio_write(pb, pkt->data, pkt->size); + + if (mkv->mode != MODE_WEBM && (id_size || settings_size || comment_size)) { + ebml_master block_additions = start_ebml_master(pb, MATROSKA_ID_BLOCKADDITIONS, 0); + ebml_master block_more = start_ebml_master(pb, MATROSKA_ID_BLOCKMORE, 0); + /* Until dbc50f8a our demuxer used a wrong default value + * of BlockAddID, so we write it unconditionally. */ + put_ebml_uint (pb, MATROSKA_ID_BLOCKADDID, 1); + put_ebml_id(pb, MATROSKA_ID_BLOCKADDITIONAL); + put_ebml_length(pb, id_size + 1 + settings_size + 1 + comment_size, 0); + avio_printf(pb, "%.*s\n%.*s\n%.*s", id_size, id, settings_size, settings, comment_size, comment); + end_ebml_master(pb, block_more); + end_ebml_master(pb, block_additions); + } put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, pkt->duration); end_ebml_master(pb, blockgroup);