diff mbox series

[FFmpeg-devel,4/5] lavf/matroskaenc: mux WebVTT using standard (non-WebM) formatting

Message ID 20200618004743.95896-4-rcombs@rcombs.me
State New
Headers show
Series [FFmpeg-devel,1/5] lavf: matroska subtitle muxer
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

rcombs June 18, 2020, 12:47 a.m. UTC
---
 libavformat/matroskaenc.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

Comments

Andreas Rheinhardt June 20, 2020, 9:12 a.m. UTC | #1
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
rcombs June 20, 2020, 9:46 a.m. UTC | #2
> 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 mbox series

Patch

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);