[FFmpeg-devel] lavf/movenc: Write 'dby1' minor brand if Dolby content is being muxed to MP4

This is as per:
   * mp4ra: http://mp4ra.org/#/brands
   * Dolby Vision muxing spec (which is public):
       https://professional.dolby.com/siteassets/content-creation/dolby-vision-for-content-creators/dolby_vision_bitstreams_within_the_iso_base_media_file_format_dec2017.pdf

Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
---
The sole FATE change is just the brand being written for EAC-3.
---
 libavformat/movenc.c         | 9 ++++++++-
 tests/ref/fate/copy-trac3074 | 4 ++--
 2 files changed, 10 insertions(+), 3 deletions(-)

On Tue, Sep 28, 2021 at 5:31 PM Derek Buitenhuis
<derek.buitenhuis@gmail.com> wrote:
>
> This is as per:
>    * mp4ra: http://mp4ra.org/#/brands
>    * Dolby Vision muxing spec (which is public):
>        https://professional.dolby.com/siteassets/content-creation/dolby-vision-for-content-creators/dolby_vision_bitstreams_within_the_iso_base_media_file_format_dec2017.pdf
>
> Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
> ---
> The sole FATE change is just the brand being written for EAC-3.

I do dislike it how outside of the DoVi pdf they don't really seem to
specify 'dby1', but the mp4 registration authority's description goes
all the way back to January 2017 with this identifier
(https://github.com/mp4ra/mp4ra.github.io/blob/a27f402652b57cea190a33f6955a843869fdd457/filetype.html).

f.ex. none of the following seem to mention the 'dby1' brand:

TrueHD/MLP in MP4
https://developer.dolby.com/globalassets/technology/dolby-truehd/dolby-truehd-mlp-bitstreams-within-the-iso-base-media-file-format.pdf

(E-)AC-3 in MP4
https://www.etsi.org/deliver/etsi_ts/102300_102399/102366/01.04.01_60/ts_102366v010401p.pdf
(E-)AC-3 with Object Based Audio in MP4
https://www.etsi.org/deliver/etsi_ts/103400_103499/103420/01.02.01_60/ts_103420v010201p.pdf

AC-4 Part 1 in MP4
https://www.etsi.org/deliver/etsi_ts/103100_103199/10319001/01.03.01_60/ts_10319001v010301p.pdf
AC-4 Part 2 in MP4
https://www.etsi.org/deliver/etsi_ts/103100_103199/10319002/01.02.01_60/ts_10319002v010201p.pdf

> ---
>  libavformat/movenc.c         | 9 ++++++++-
>  tests/ref/fate/copy-trac3074 | 4 ++--
>  2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> index 7650ac5ed3..fe3405d271 100644
> --- a/libavformat/movenc.c
> +++ b/libavformat/movenc.c
> @@ -4991,11 +4991,13 @@ static int mov_write_ftyp_tag(AVIOContext *pb, AVFormatContext *s)
>  {
>      MOVMuxContext *mov = s->priv_data;
>      int64_t pos = avio_tell(pb);
> -    int has_h264 = 0, has_av1 = 0, has_video = 0;
> +    int has_h264 = 0, has_av1 = 0, has_video = 0, has_dolby = 0;
>      int i;
>
>      for (i = 0; i < s->nb_streams; i++) {
>          AVStream *st = s->streams[i];
> +        AVDOVIDecoderConfigurationRecord *dovi = (AVDOVIDecoderConfigurationRecord *)
> +                                                     av_stream_get_side_data(st, AV_PKT_DATA_DOVI_CONF, NULL);

Maybe something a la the following to keep the line length shorter?

+        AVDOVIDecoderConfigurationRecord *dovi =
+            (AVDOVIDecoderConfigurationRecord *)
+            av_stream_get_side_data(st, AV_PKT_DATA_DOVI_CONF, NULL);
+

>          if (is_cover_image(st))
>              continue;
>          if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO)
> @@ -5004,6 +5006,9 @@ static int mov_write_ftyp_tag(AVIOContext *pb, AVFormatContext *s)
>              has_h264 = 1;
>          if (st->codecpar->codec_id == AV_CODEC_ID_AV1)
>              has_av1 = 1;
> +        if (dovi || st->codecpar->codec_id == AV_CODEC_ID_AC3 ||
> +            st->codecpar->codec_id == AV_CODEC_ID_EAC3 || st->codecpar->codec_id == AV_CODEC_ID_TRUEHD)
> +            has_dolby = 1;

Maybe something a la the following to keep the line length shorter
(and also the codec_id checks aligned)?
+        if (dovi ||
+            st->codecpar->codec_id == AV_CODEC_ID_AC3 ||
+            st->codecpar->codec_id == AV_CODEC_ID_EAC3 ||
+            st->codecpar->codec_id == AV_CODEC_ID_TRUEHD)

(I think the next step around this code would almost be a switch/case
thing since we've got multiple of them now :D)

Otherwise LGTM.

Jan

Derek Buitenhuis:
> This is as per:
>    * mp4ra: http://mp4ra.org/#/brands
>    * Dolby Vision muxing spec (which is public):
>        https://professional.dolby.com/siteassets/content-creation/dolby-vision-for-content-creators/dolby_vision_bitstreams_within_the_iso_base_media_file_format_dec2017.pdf
> 
> Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
> ---
> The sole FATE change is just the brand being written for EAC-3.
> ---
>  libavformat/movenc.c         | 9 ++++++++-
>  tests/ref/fate/copy-trac3074 | 4 ++--
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> index 7650ac5ed3..fe3405d271 100644
> --- a/libavformat/movenc.c
> +++ b/libavformat/movenc.c
> @@ -4991,11 +4991,13 @@ static int mov_write_ftyp_tag(AVIOContext *pb, AVFormatContext *s)
>  {
>      MOVMuxContext *mov = s->priv_data;
>      int64_t pos = avio_tell(pb);
> -    int has_h264 = 0, has_av1 = 0, has_video = 0;
> +    int has_h264 = 0, has_av1 = 0, has_video = 0, has_dolby = 0;
>      int i;
>  
>      for (i = 0; i < s->nb_streams; i++) {
>          AVStream *st = s->streams[i];
> +        AVDOVIDecoderConfigurationRecord *dovi = (AVDOVIDecoderConfigurationRecord *)
> +                                                     av_stream_get_side_data(st, AV_PKT_DATA_DOVI_CONF, NULL);

If you checked for the existence of this side data later (see below),
you would increase code locality and could avoid the dovi variable (and
the huge cast) altogether.

>          if (is_cover_image(st))
>              continue;
>          if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO)
> @@ -5004,6 +5006,9 @@ static int mov_write_ftyp_tag(AVIOContext *pb, AVFormatContext *s)
>              has_h264 = 1;
>          if (st->codecpar->codec_id == AV_CODEC_ID_AV1)
>              has_av1 = 1;
> +        if (dovi || st->codecpar->codec_id == AV_CODEC_ID_AC3 ||
> +            st->codecpar->codec_id == AV_CODEC_ID_EAC3 || st->codecpar->codec_id == AV_CODEC_ID_TRUEHD)

if (st->codecpar->codec_id == AV_CODEC_ID_AC3    ||

    st->codecpar->codec_id == AV_CODEC_ID_EAC3   ||

    st->codecpar->codec_id == AV_CODEC_ID_TRUEHD ||

    av_stream_get_side_data(st, AV_PKT_DATA_DOVI_CONF, NULL))

    has_dolby = 1;


> +            has_dolby = 1;
>      }
>  
>      avio_wb32(pb, 0); /* size */
> @@ -5029,6 +5034,8 @@ static int mov_write_ftyp_tag(AVIOContext *pb, AVFormatContext *s)
>                  ffio_wfourcc(pb, "iso6");
>              if (has_av1)
>                  ffio_wfourcc(pb, "av01");
> +            if (has_dolby)
> +                ffio_wfourcc(pb, "dby1");
>          } else {
>              if (mov->flags & FF_MOV_FLAG_FRAGMENT)
>                  ffio_wfourcc(pb, "iso6");
> diff --git a/tests/ref/fate/copy-trac3074 b/tests/ref/fate/copy-trac3074
> index e541af03da..4748296c2a 100644
> --- a/tests/ref/fate/copy-trac3074
> +++ b/tests/ref/fate/copy-trac3074
> @@ -1,5 +1,5 @@
> -da6122873fb83ce4340cf5d0ab8d475e *tests/data/fate/copy-trac3074.mp4
> -334012 tests/data/fate/copy-trac3074.mp4
> +452d91e7c6889b787717fef25b6fce43 *tests/data/fate/copy-trac3074.mp4
> +334016 tests/data/fate/copy-trac3074.mp4
>  #tb 0: 1/48000
>  #media_type 0: audio
>  #codec_id 0: eac3
>

On 9/29/2021 8:30 PM, Jan Ekström wrote:
> I do dislike it how outside of the DoVi pdf they don't really seem to
> specify 'dby1', but the mp4 registration authority's description goes
> all the way back to January 2017 with this identifier
> (https://github.com/mp4ra/mp4ra.github.io/blob/a27f402652b57cea190a33f6955a843869fdd457/filetype.html).
> 
> f.ex. none of the following seem to mention the 'dby1' brand:

I know, but I am inclined to follow the MP4A here.

>> +        AVDOVIDecoderConfigurationRecord *dovi = (AVDOVIDecoderConfigurationRecord *)
>> +                                                     av_stream_get_side_data(st, AV_PKT_DATA_DOVI_CONF, NULL);
> 
> Maybe something a la the following to keep the line length shorter?

[...]

>> +        if (dovi || st->codecpar->codec_id == AV_CODEC_ID_AC3 ||
>> +            st->codecpar->codec_id == AV_CODEC_ID_EAC3 || st->codecpar->codec_id == AV_CODEC_ID_TRUEHD)
>> +            has_dolby = 1;
> 
> Maybe something a la the following to keep the line length shorter
> (and also the codec_id checks aligned)?
> +        if (dovi ||
> +            st->codecpar->codec_id == AV_CODEC_ID_AC3 ||
> +            st->codecpar->codec_id == AV_CODEC_ID_EAC3 ||
> +            st->codecpar->codec_id == AV_CODEC_ID_TRUEHD)

I've changed it to what Andreas sugested in v2, which should
satisify this.

> (I think the next step around this code would almost be a switch/case
> thing since we've got multiple of them now :D)
> 
> Otherwise LGTM.

v2 sent.

- Derek

On 9/30/2021 12:43 AM, Andreas Rheinhardt wrote:
>> +        AVDOVIDecoderConfigurationRecord *dovi = (AVDOVIDecoderConfigurationRecord *)
>> +                                                     av_stream_get_side_data(st, AV_PKT_DATA_DOVI_CONF, NULL);
> 
> If you checked for the existence of this side data later (see below),
> you would increase code locality and could avoid the dovi variable (and
> the huge cast) altogether.

Changed in v2.

- Derek