[FFmpeg-devel,v2] avformat/matroskaenc: Write level 1 elements in one go

Up until now, writing level 1 elements proceeded as follows: First, the
element id was written to the ordinary output AVIOContext and a dynamic
buffer was opened for the content of the level 1 element in
start_ebml_master_crc32(). Then this buffer was actually used and after it
was closed (in end_ebml_master_crc32()), the size field corresponding to
the buffer's size was written, after which the actual data was written.

This commit changes this: Nothing is written to the main AVIOContext any
more in start_ebml_master_crc32(). end_ebml_master_crc32() now writes
both the id, the length field as well as the data. This implies that
one can start a level 1 element in memory without outputting anything.
This is done to enable to test whether enough space has been reserved
for the Cues (if space has been reserved for them) before writing them.
A large duration between outputting the header and outputting the rest
could also break certain streaming usecases like the one from #8578
(which this commit fixes).

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
The commit message has been updated in light of ticket #8578; nothing
else has changed.

I'd like to apply this and everything until (but excluding) #18 [1]
in the coming days to fix the aforementioned issue (I will also backport
a fix to 4.2).

[1] is the cutoff point because upon rereading I am not sure anymore
whether it is good to write the cues at the end if the reserved space
doesn't suffice and return normally afterwards, because the fact that
the cues have not been written at the front won't be detectable from the
return value of av_write_trailer() any more (one only gets a
AV_LOG_WARNING logmessage).

[1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-January/255157.html

 libavformat/matroskaenc.c | 60 +++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 28 deletions(-)

Am Mi., 25. März 2020 um 05:45 Uhr schrieb Andreas Rheinhardt
<andreas.rheinhardt@gmail.com>:
>
> Up until now, writing level 1 elements proceeded as follows: First, the
> element id was written to the ordinary output AVIOContext and a dynamic
> buffer was opened for the content of the level 1 element in
> start_ebml_master_crc32(). Then this buffer was actually used and after it
> was closed (in end_ebml_master_crc32()), the size field corresponding to
> the buffer's size was written, after which the actual data was written.
>
> This commit changes this: Nothing is written to the main AVIOContext any
> more in start_ebml_master_crc32(). end_ebml_master_crc32() now writes
> both the id, the length field as well as the data. This implies that
> one can start a level 1 element in memory without outputting anything.
> This is done to enable to test whether enough space has been reserved
> for the Cues (if space has been reserved for them) before writing them.
> A large duration between outputting the header and outputting the rest
> could also break certain streaming usecases like the one from #8578
> (which this commit fixes).
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> The commit message has been updated in light of ticket #8578; nothing
> else has changed.

Please mention the ticket in the commit message.

Thank you, Carl Eugen

On 3/25/2020 1:44 AM, Andreas Rheinhardt wrote:
> Up until now, writing level 1 elements proceeded as follows: First, the
> element id was written to the ordinary output AVIOContext and a dynamic
> buffer was opened for the content of the level 1 element in
> start_ebml_master_crc32(). Then this buffer was actually used and after it
> was closed (in end_ebml_master_crc32()), the size field corresponding to
> the buffer's size was written, after which the actual data was written.
> 
> This commit changes this: Nothing is written to the main AVIOContext any
> more in start_ebml_master_crc32(). end_ebml_master_crc32() now writes
> both the id, the length field as well as the data. This implies that
> one can start a level 1 element in memory without outputting anything.
> This is done to enable to test whether enough space has been reserved
> for the Cues (if space has been reserved for them) before writing them.
> A large duration between outputting the header and outputting the rest
> could also break certain streaming usecases like the one from #8578
> (which this commit fixes).
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> The commit message has been updated in light of ticket #8578; nothing
> else has changed.

Should be ok. And please backport it to release/4.2 branch using git
cherry-pick -x, so the commit hash from master is referenced.

> 
> I'd like to apply this and everything until (but excluding) #18 [1]
> in the coming days to fix the aforementioned issue (I will also backport
> a fix to 4.2).
> 
> [1] is the cutoff point because upon rereading I am not sure anymore
> whether it is good to write the cues at the end if the reserved space
> doesn't suffice and return normally afterwards, because the fact that
> the cues have not been written at the front won't be detectable from the
> return value of av_write_trailer() any more (one only gets a
> AV_LOG_WARNING logmessage).
> 
> [1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-January/255157.html
> 
>  libavformat/matroskaenc.c | 60 +++++++++++++++++++++------------------
>  1 file changed, 32 insertions(+), 28 deletions(-)
> 
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index 66f1bc4255..501d68a8f7 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -325,26 +325,26 @@ static void end_ebml_master(AVIOContext *pb, ebml_master master)
>      avio_seek(pb, pos, SEEK_SET);
>  }
>  
> -static int start_ebml_master_crc32(AVIOContext *pb, AVIOContext **dyn_cp, MatroskaMuxContext *mkv,
> -                                   uint32_t elementid)
> +static int start_ebml_master_crc32(AVIOContext **dyn_cp, MatroskaMuxContext *mkv)
>  {
>      int ret;
>  
>      if ((ret = avio_open_dyn_buf(dyn_cp)) < 0)
>          return ret;
>  
> -    put_ebml_id(pb, elementid);
>      if (mkv->write_crc)
>          put_ebml_void(*dyn_cp, 6); /* Reserve space for CRC32 so position/size calculations using avio_tell() take it into account */
>  
>      return 0;
>  }
>  
> -static void end_ebml_master_crc32(AVIOContext *pb, AVIOContext **dyn_cp, MatroskaMuxContext *mkv)
> +static void end_ebml_master_crc32(AVIOContext *pb, AVIOContext **dyn_cp,
> +                                  MatroskaMuxContext *mkv, uint32_t id)
>  {
>      uint8_t *buf, crc[4];
>      int size, skip = 0;
>  
> +    put_ebml_id(pb, id);
>      size = avio_close_dyn_buf(*dyn_cp, &buf);
>      put_ebml_num(pb, size, 0);
>      if (mkv->write_crc) {
> @@ -362,13 +362,14 @@ static void end_ebml_master_crc32(AVIOContext *pb, AVIOContext **dyn_cp, Matrosk
>  * Complete ebml master without destroying the buffer, allowing for later updates
>  */
>  static void end_ebml_master_crc32_preliminary(AVIOContext *pb, AVIOContext **dyn_cp, MatroskaMuxContext *mkv,
> -                                              int64_t *pos)
> +                                              uint32_t id, int64_t *pos)
>  {
>      uint8_t *buf;
>      int size = avio_get_dyn_buf(*dyn_cp, &buf);
>  
>      *pos = avio_tell(pb);
>  
> +    put_ebml_id(pb, id);
>      put_ebml_num(pb, size, 0);
>      avio_write(pb, buf, size);
>  }
> @@ -450,7 +451,7 @@ static int mkv_write_seekhead(AVIOContext *pb, MatroskaMuxContext *mkv,
>          goto seek;
>      }
>  
> -    ret = start_ebml_master_crc32(pb, &dyn_cp, mkv, MATROSKA_ID_SEEKHEAD);
> +    ret = start_ebml_master_crc32(&dyn_cp, mkv);
>      if (ret < 0)
>          return ret;
>  
> @@ -466,7 +467,7 @@ static int mkv_write_seekhead(AVIOContext *pb, MatroskaMuxContext *mkv,
>          put_ebml_uint(dyn_cp, MATROSKA_ID_SEEKPOSITION, entry->segmentpos);
>          end_ebml_master(dyn_cp, seekentry);
>      }
> -    end_ebml_master_crc32(pb, &dyn_cp, mkv);
> +    end_ebml_master_crc32(pb, &dyn_cp, mkv, MATROSKA_ID_SEEKHEAD);
>  
>      remaining = seekhead->filepos + seekhead->reserved_size - avio_tell(pb);
>      put_ebml_void(pb, remaining);
> @@ -510,7 +511,7 @@ static int64_t mkv_write_cues(AVFormatContext *s, mkv_cues *cues, mkv_track *tra
>      int ret;
>  
>      currentpos = avio_tell(pb);
> -    ret = start_ebml_master_crc32(pb, &dyn_cp, mkv, MATROSKA_ID_CUES);
> +    ret = start_ebml_master_crc32(&dyn_cp, mkv);
>      if (ret < 0)
>          return ret;
>  
> @@ -553,7 +554,7 @@ static int64_t mkv_write_cues(AVFormatContext *s, mkv_cues *cues, mkv_track *tra
>          ffio_reset_dyn_buf(cuepoint);
>      }
>      ffio_free_dyn_buf(&cuepoint);
> -    end_ebml_master_crc32(pb, &dyn_cp, mkv);
> +    end_ebml_master_crc32(pb, &dyn_cp, mkv, MATROSKA_ID_CUES);
>  
>      return currentpos;
>  }
> @@ -1375,7 +1376,7 @@ static int mkv_write_tracks(AVFormatContext *s)
>  
>      mkv_add_seekhead_entry(mkv, MATROSKA_ID_TRACKS, avio_tell(pb));
>  
> -    ret = start_ebml_master_crc32(pb, &mkv->tracks_bc, mkv, MATROSKA_ID_TRACKS);
> +    ret = start_ebml_master_crc32(&mkv->tracks_bc, mkv);
>      if (ret < 0)
>          return ret;
>  
> @@ -1390,9 +1391,10 @@ static int mkv_write_tracks(AVFormatContext *s)
>      }
>  
>      if ((pb->seekable & AVIO_SEEKABLE_NORMAL) && !mkv->is_live)
> -        end_ebml_master_crc32_preliminary(pb, &mkv->tracks_bc, mkv, &mkv->tracks_pos);
> +        end_ebml_master_crc32_preliminary(pb, &mkv->tracks_bc, mkv,
> +                                          MATROSKA_ID_TRACKS, &mkv->tracks_pos);
>      else
> -        end_ebml_master_crc32(pb, &mkv->tracks_bc, mkv);
> +        end_ebml_master_crc32(pb, &mkv->tracks_bc, mkv, MATROSKA_ID_TRACKS);
>  
>      return 0;
>  }
> @@ -1410,7 +1412,7 @@ static int mkv_write_chapters(AVFormatContext *s)
>  
>      mkv_add_seekhead_entry(mkv, MATROSKA_ID_CHAPTERS, avio_tell(pb));
>  
> -    ret = start_ebml_master_crc32(pb, &dyn_cp, mkv, MATROSKA_ID_CHAPTERS);
> +    ret = start_ebml_master_crc32(&dyn_cp, mkv);
>      if (ret < 0) return ret;
>  
>      editionentry = start_ebml_master(dyn_cp, MATROSKA_ID_EDITIONENTRY, 0);
> @@ -1448,7 +1450,7 @@ static int mkv_write_chapters(AVFormatContext *s)
>          end_ebml_master(dyn_cp, chapteratom);
>      }
>      end_ebml_master(dyn_cp, editionentry);
> -    end_ebml_master_crc32(pb, &dyn_cp, mkv);
> +    end_ebml_master_crc32(pb, &dyn_cp, mkv, MATROSKA_ID_CHAPTERS);
>  
>      mkv->wrote_chapters = 1;
>      return 0;
> @@ -1499,7 +1501,7 @@ static int mkv_write_tag_targets(AVFormatContext *s, uint32_t elementid,
>      if (!mkv->tags_bc) {
>          mkv_add_seekhead_entry(mkv, MATROSKA_ID_TAGS, avio_tell(s->pb));
>  
> -        ret = start_ebml_master_crc32(s->pb, &mkv->tags_bc, mkv, MATROSKA_ID_TAGS);
> +        ret = start_ebml_master_crc32(&mkv->tags_bc, mkv);
>          if (ret < 0)
>              return ret;
>      }
> @@ -1644,9 +1646,10 @@ static int mkv_write_tags(AVFormatContext *s)
>  
>      if (mkv->tags_bc) {
>          if ((s->pb->seekable & AVIO_SEEKABLE_NORMAL) && !mkv->is_live)
> -            end_ebml_master_crc32_preliminary(s->pb, &mkv->tags_bc, mkv, &mkv->tags_pos);
> +            end_ebml_master_crc32_preliminary(s->pb, &mkv->tags_bc, mkv,
> +                                              MATROSKA_ID_TAGS, &mkv->tags_pos);
>          else
> -            end_ebml_master_crc32(s->pb, &mkv->tags_bc, mkv);
> +            end_ebml_master_crc32(s->pb, &mkv->tags_bc, mkv, MATROSKA_ID_TAGS);
>      }
>      return 0;
>  }
> @@ -1669,7 +1672,7 @@ static int mkv_write_attachments(AVFormatContext *s)
>  
>      mkv_add_seekhead_entry(mkv, MATROSKA_ID_ATTACHMENTS, avio_tell(pb));
>  
> -    ret = start_ebml_master_crc32(pb, &dyn_cp, mkv, MATROSKA_ID_ATTACHMENTS);
> +    ret = start_ebml_master_crc32(&dyn_cp, mkv);
>      if (ret < 0) return ret;
>  
>      for (i = 0; i < s->nb_streams; i++) {
> @@ -1742,7 +1745,7 @@ static int mkv_write_attachments(AVFormatContext *s)
>          mkv->attachments->entries[mkv->attachments->num_entries].stream_idx = i;
>          mkv->attachments->entries[mkv->attachments->num_entries++].fileuid  = fileuid;
>      }
> -    end_ebml_master_crc32(pb, &dyn_cp, mkv);
> +    end_ebml_master_crc32(pb, &dyn_cp, mkv, MATROSKA_ID_ATTACHMENTS);
>  
>      return 0;
>  }
> @@ -1821,7 +1824,7 @@ static int mkv_write_header(AVFormatContext *s)
>  
>      mkv_add_seekhead_entry(mkv, MATROSKA_ID_INFO, avio_tell(pb));
>  
> -    ret = start_ebml_master_crc32(pb, &mkv->info_bc, mkv, MATROSKA_ID_INFO);
> +    ret = start_ebml_master_crc32(&mkv->info_bc, mkv);
>      if (ret < 0)
>          return ret;
>      pb = mkv->info_bc;
> @@ -1880,9 +1883,10 @@ static int mkv_write_header(AVFormatContext *s)
>          }
>      }
>      if ((s->pb->seekable & AVIO_SEEKABLE_NORMAL) && !mkv->is_live)
> -        end_ebml_master_crc32_preliminary(s->pb, &mkv->info_bc, mkv, &mkv->info_pos);
> +        end_ebml_master_crc32_preliminary(s->pb, &mkv->info_bc, mkv,
> +                                          MATROSKA_ID_INFO, &mkv->info_pos);
>      else
> -        end_ebml_master_crc32(s->pb, &mkv->info_bc, mkv);
> +        end_ebml_master_crc32(s->pb, &mkv->info_bc, mkv, MATROSKA_ID_INFO);
>      pb = s->pb;
>  
>      ret = mkv_write_tracks(s);
> @@ -2170,7 +2174,7 @@ static void mkv_end_cluster(AVFormatContext *s)
>  {
>      MatroskaMuxContext *mkv = s->priv_data;
>  
> -    end_ebml_master_crc32(s->pb, &mkv->cluster_bc, mkv);
> +    end_ebml_master_crc32(s->pb, &mkv->cluster_bc, mkv, MATROSKA_ID_CLUSTER);
>      mkv->cluster_pos = -1;
>      avio_write_marker(s->pb, AV_NOPTS_VALUE, AVIO_DATA_MARKER_FLUSH_POINT);
>  }
> @@ -2311,7 +2315,7 @@ static int mkv_write_packet_internal(AVFormatContext *s, AVPacket *pkt, int add_
>  
>      if (mkv->cluster_pos == -1) {
>          mkv->cluster_pos = avio_tell(s->pb);
> -        ret = start_ebml_master_crc32(s->pb, &mkv->cluster_bc, mkv, MATROSKA_ID_CLUSTER);
> +        ret = start_ebml_master_crc32(&mkv->cluster_bc, mkv);
>          if (ret < 0)
>              return ret;
>          put_ebml_uint(mkv->cluster_bc, MATROSKA_ID_CLUSTERTIMECODE, FFMAX(0, ts));
> @@ -2477,7 +2481,7 @@ static int mkv_write_trailer(AVFormatContext *s)
>      }
>  
>      if (mkv->cluster_bc) {
> -        end_ebml_master_crc32(pb, &mkv->cluster_bc, mkv);
> +        end_ebml_master_crc32(pb, &mkv->cluster_bc, mkv, MATROSKA_ID_CLUSTER);
>      }
>  
>      ret = mkv_write_chapters(s);
> @@ -2525,11 +2529,11 @@ static int mkv_write_trailer(AVFormatContext *s)
>          av_log(s, AV_LOG_DEBUG, "end duration = %" PRIu64 "\n", mkv->duration);
>          avio_seek(mkv->info_bc, mkv->duration_offset, SEEK_SET);
>          put_ebml_float(mkv->info_bc, MATROSKA_ID_DURATION, mkv->duration);
> -        end_ebml_master_crc32(pb, &mkv->info_bc, mkv);
> +        end_ebml_master_crc32(pb, &mkv->info_bc, mkv, MATROSKA_ID_INFO);
>  
>          // write tracks master
>          avio_seek(pb, mkv->tracks_pos, SEEK_SET);
> -        end_ebml_master_crc32(pb, &mkv->tracks_bc, mkv);
> +        end_ebml_master_crc32(pb, &mkv->tracks_bc, mkv, MATROSKA_ID_TRACKS);
>  
>          // update stream durations
>          if (!mkv->is_live) {
> @@ -2559,7 +2563,7 @@ static int mkv_write_trailer(AVFormatContext *s)
>          }
>          if (mkv->tags_bc && !mkv->is_live) {
>              avio_seek(pb, mkv->tags_pos, SEEK_SET);
> -            end_ebml_master_crc32(pb, &mkv->tags_bc, mkv);
> +            end_ebml_master_crc32(pb, &mkv->tags_bc, mkv, MATROSKA_ID_TAGS);
>          }
>  
>          avio_seek(pb, currentpos, SEEK_SET);
>

Carl Eugen Hoyos:
> Am Mi., 25. März 2020 um 05:45 Uhr schrieb Andreas Rheinhardt
> <andreas.rheinhardt@gmail.com>:
>>
>> Up until now, writing level 1 elements proceeded as follows: First, the
>> element id was written to the ordinary output AVIOContext and a dynamic
>> buffer was opened for the content of the level 1 element in
>> start_ebml_master_crc32(). Then this buffer was actually used and after it
>> was closed (in end_ebml_master_crc32()), the size field corresponding to
>> the buffer's size was written, after which the actual data was written.
>>
>> This commit changes this: Nothing is written to the main AVIOContext any
>> more in start_ebml_master_crc32(). end_ebml_master_crc32() now writes
>> both the id, the length field as well as the data. This implies that
>> one can start a level 1 element in memory without outputting anything.
>> This is done to enable to test whether enough space has been reserved
>> for the Cues (if space has been reserved for them) before writing them.
>> A large duration between outputting the header and outputting the rest
>> could also break certain streaming usecases like the one from #8578
>> (which this commit fixes).
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>> The commit message has been updated in light of ticket #8578; nothing
>> else has changed.
> 
> Please mention the ticket in the commit message.
> 
It is already contained in the commit message.

- Andreas

Am Mi., 25. März 2020 um 18:08 Uhr schrieb Andreas Rheinhardt
<andreas.rheinhardt@gmail.com>:
>
> Carl Eugen Hoyos:
> > Am Mi., 25. März 2020 um 05:45 Uhr schrieb Andreas Rheinhardt
> > <andreas.rheinhardt@gmail.com>:
> >>
> >> Up until now, writing level 1 elements proceeded as follows: First, the
> >> element id was written to the ordinary output AVIOContext and a dynamic
> >> buffer was opened for the content of the level 1 element in
> >> start_ebml_master_crc32(). Then this buffer was actually used and after it
> >> was closed (in end_ebml_master_crc32()), the size field corresponding to
> >> the buffer's size was written, after which the actual data was written.
> >>
> >> This commit changes this: Nothing is written to the main AVIOContext any
> >> more in start_ebml_master_crc32(). end_ebml_master_crc32() now writes
> >> both the id, the length field as well as the data. This implies that
> >> one can start a level 1 element in memory without outputting anything.
> >> This is done to enable to test whether enough space has been reserved
> >> for the Cues (if space has been reserved for them) before writing them.
> >> A large duration between outputting the header and outputting the rest
> >> could also break certain streaming usecases like the one from #8578
> >> (which this commit fixes).
> >>
> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> >> ---
> >> The commit message has been updated in light of ticket #8578; nothing
> >> else has changed.
> >
> > Please mention the ticket in the commit message.
> >
> It is already contained in the commit message.

Found it.

Sorry, Carl Eugen