diff mbox series

[FFmpeg-devel,v2] avformat/movenc: write the colr atom by default

Message ID CAHUoETLrPyA5ixwG3TVckqemG8nTyYOU4K_zhL2Nt8m+Z8hVVA@mail.gmail.com
State Superseded
Headers show
Series [FFmpeg-devel,v2] avformat/movenc: write the colr atom by default | expand

Checks

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

Commit Message

Michael Bradshaw April 13, 2020, 5:58 p.m. UTC 
At the risk of being too non-committal, attached patch is mostly the same
but leaves the write_colr flag as experimental. I would love to remove the
write_colr flag entirely but we should provide some kind of escape hatch to
allow ffmpeg to write "unspecified" (enum value 2) values for the colr atom
(after all, these are legitimate values in H.273). Here are the reasons I'm
not sold on stabilizing write_colr right now:

   - It only fixes mov files. mkv has the same problem. It would be nice to
   stabilize a solution that isn't per-format.
   - I'm not motivated to spend the time exploring and debating alternative
   solutions like signaling "unspecified" (enum value 2) values differently
   from "unset" (no value from H.273) somehow (e.g., use enum value -1, or add
   a separate bool flag to signal whether the values are just ffmpeg defaults
   or not).
   - This approach solves the use cases with which I'm most familiar. I'm
   not as familiar with situations where people want to write "unspecified"
   enum values in the colr atom. I'd rather leave this for other people (who
   hopefully have more experience (and opinions) on this than I do).

Comments

Michael Bradshaw April 13, 2020, 10:14 p.m. UTC | #1 
Attached is an updated patch that passes fate.
diff mbox series

Patch

From d6fcb5705d2c451b9ec1146b4fe66517f9eb6692 Mon Sep 17 00:00:00 2001
From: Michael Bradshaw <mjbshaw@google.com>
Date: Mon, 13 Apr 2020 11:11:38 -0600
Subject: [PATCH] avformat/movenc: write the colr atom by default

The write_colr flag has been marked as experimental for over 5 years.
It should be safe to enable its behavior by default as follows:

  - Write the colr atom by default for mp4/mov if any of the following:
     - The primaries/trc/matrix are all specified, OR
     - There is an ICC profile, OR
     - The user specified +write_colr
  - Keep the write_colr flag for situations where the user wants to
    write the colr atom even if the color info is unspecified (e.g.,
    http://ffmpeg.org/pipermail/ffmpeg-devel/2020-March/259334.html)

This fixes https://trac.ffmpeg.org/ticket/7961

Signed-off-by: Michael Bradshaw <mjbshaw@google.com>
---
 libavformat/movenc.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index bc8d08044e..4a4c9ce481 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -77,7 +77,7 @@  static const AVOption options[] = {
     { "delay_moov", "Delay writing the initial moov until the first fragment is cut, or until the first fragment flush", 0, AV_OPT_TYPE_CONST, {.i64 = FF_MOV_FLAG_DELAY_MOOV}, INT_MIN, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM, "movflags" },
     { "global_sidx", "Write a global sidx index at the start of the file", 0, AV_OPT_TYPE_CONST, {.i64 = FF_MOV_FLAG_GLOBAL_SIDX}, INT_MIN, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM, "movflags" },
     { "skip_sidx", "Skip writing of sidx atom", 0, AV_OPT_TYPE_CONST, {.i64 = FF_MOV_FLAG_SKIP_SIDX}, INT_MIN, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM, "movflags" },
-    { "write_colr", "Write colr atom (Experimental, may be renamed or changed, do not use from scripts)", 0, AV_OPT_TYPE_CONST, {.i64 = FF_MOV_FLAG_WRITE_COLR}, INT_MIN, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM, "movflags" },
+    { "write_colr", "Write colr atom even if the color info is unspecified (Experimental, may be renamed or changed, do not use from scripts)", 0, AV_OPT_TYPE_CONST, {.i64 = FF_MOV_FLAG_WRITE_COLR}, INT_MIN, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM, "movflags" },
     { "prefer_icc", "If writing colr atom prioritise usage of ICC profile if it exists in stream packet side data", 0, AV_OPT_TYPE_CONST, {.i64 = FF_MOV_FLAG_PREFER_ICC}, INT_MIN, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM, "movflags" },
     { "write_gama", "Write deprecated gama atom", 0, AV_OPT_TYPE_CONST, {.i64 = FF_MOV_FLAG_WRITE_GAMA}, INT_MIN, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM, "movflags" },
     { "use_metadata_tags", "Use mdta atom for metadata.", 0, AV_OPT_TYPE_CONST, {.i64 = FF_MOV_FLAG_USE_MDTA}, INT_MIN, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM, "movflags" },
@@ -2135,11 +2135,17 @@  static int mov_write_video_tag(AVFormatContext *s, AVIOContext *pb, MOVMuxContex
         else
             av_log(mov->fc, AV_LOG_WARNING, "Not writing 'gama' atom. Format is not MOV.\n");
     }
-    if (mov->flags & FF_MOV_FLAG_WRITE_COLR) {
-        if (track->mode == MODE_MOV || track->mode == MODE_MP4)
-            mov_write_colr_tag(pb, track, mov->flags & FF_MOV_FLAG_PREFER_ICC);
-        else
-            av_log(mov->fc, AV_LOG_WARNING, "Not writing 'colr' atom. Format is not MOV or MP4.\n");
+    if (track->mode == MODE_MOV || track->mode == MODE_MP4) {
+        int has_color_info = track->par->color_primaries != AVCOL_PRI_UNSPECIFIED &&
+                             track->par->color_trc != AVCOL_TRC_UNSPECIFIED &&
+                             track->par->color_space != AVCOL_SPC_UNSPECIFIED;
+        if (has_color_info || mov->flags & FF_MOV_FLAG_WRITE_COLR ||
+            av_stream_get_side_data(track->st, AV_PKT_DATA_ICC_PROFILE, NULL)) {
+            int prefer_icc = mov->flags & FF_MOV_FLAG_PREFER_ICC || !has_color_info;
+            mov_write_colr_tag(pb, track, prefer_icc);
+        } else if (mov->flags & FF_MOV_FLAG_WRITE_COLR) {
+             av_log(mov->fc, AV_LOG_WARNING, "Not writing 'colr' atom. Format is not MOV or MP4.\n");
+        }
     }
     if (track->mode == MODE_MOV || track->mode == MODE_MP4) {
         mov_write_clli_tag(pb, track);
-- 
2.24.0.525.g8f36a354ae-goog