[FFmpeg-devel] libavformat/mov: Accept known codepoints in 'colr'

This change relaxes the whitelist on reading color metadata in MOV/BMFF
containers. The whitelist on writing values is still in place.

As a consequence it also fixes an apparent bug in reading 'nclc' values.
The 'nclc' spec [1] is in harmony with ISO 23001-8 for the values it
lists, but the code getting removed was remapping 5->6 and 6->7 for
primaries, which is incorrect, and was remapping 6->5 for color matrix
("colorspace" in the code), which is equivalent but an unnecessary
inconsistency. This logic error doesn't appear in movenc.

Removing the whitelist allows proper conversion when the source codec
relies on the container for proper signaling of newer codepoints, such
as DNxHR and VP9. If converting to a codec or container that has updated
its spec to include the new codepoints, the metadata will be preserved.
If going back to MOV/BMFF, the output whitelist will still kick in, so
this won't result in out-of-spec files being created.

[1] https://developer.apple.com/library/mac/technotes/tn2162/_index.html

Signed-off-by: Steven Robertson <steven@strobe.cc>
---
 libavformat/mov.c | 40 +++++++++-------------------------------
 1 file changed, 9 insertions(+), 31 deletions(-)

--
2.8.0.rc2

Hey folks,

We anticipate professional video software will start producing files with
the expanded codepoints Real Soon Now, and ProRes already supports Rec.
2020 and ST 2084 (see SMPTE RDD 36), so presumably MOV will get some of
that soon too. (QuickTime in OS X El Cap already recognizes Rec. 2020 color
in MOV, even though TN 2162 hasn't been updated yet.) Also it'd be nice to
land this for the bugfix in SD. Anything I can do to help with the review?

Thanks!
Steve

On Wed, Aug 17, 2016 at 12:25 AM, Steven Robertson <steven@strobe.cc> wrote:

> This change relaxes the whitelist on reading color metadata in MOV/BMFF
> containers. The whitelist on writing values is still in place.
>
> As a consequence it also fixes an apparent bug in reading 'nclc' values.
> The 'nclc' spec [1] is in harmony with ISO 23001-8 for the values it
> lists, but the code getting removed was remapping 5->6 and 6->7 for
> primaries, which is incorrect, and was remapping 6->5 for color matrix
> ("colorspace" in the code), which is equivalent but an unnecessary
> inconsistency. This logic error doesn't appear in movenc.
>
> Removing the whitelist allows proper conversion when the source codec
> relies on the container for proper signaling of newer codepoints, such
> as DNxHR and VP9. If converting to a codec or container that has updated
> its spec to include the new codepoints, the metadata will be preserved.
> If going back to MOV/BMFF, the output whitelist will still kick in, so
> this won't result in out-of-spec files being created.
>
> [1] https://developer.apple.com/library/mac/technotes/tn2162/_index.html
>
> Signed-off-by: Steven Robertson <steven@strobe.cc>
> ---
>  libavformat/mov.c | 40 +++++++++-------------------------------
>  1 file changed, 9 insertions(+), 31 deletions(-)
>
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 7c8f784..6450d52 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -1324,38 +1324,16 @@ static int mov_read_colr(MOVContext *c,
> AVIOContext *pb, MOVAtom atom)
>              st->codecpar->color_range = AVCOL_RANGE_JPEG;
>          else
>              st->codecpar->color_range = AVCOL_RANGE_MPEG;
> -        /* 14496-12 references JPEG XR specs (rather than the more
> complete
> -         * 23001-8) so some adjusting is required */
> -        if (color_primaries >= AVCOL_PRI_FILM)
> -            color_primaries = AVCOL_PRI_UNSPECIFIED;
> -        if ((color_trc >= AVCOL_TRC_LINEAR &&
> -             color_trc <= AVCOL_TRC_LOG_SQRT) ||
> -            color_trc >= AVCOL_TRC_BT2020_10)
> -            color_trc = AVCOL_TRC_UNSPECIFIED;
> -        if (color_matrix >= AVCOL_SPC_BT2020_NCL)
> -            color_matrix = AVCOL_SPC_UNSPECIFIED;
> -        st->codecpar->color_primaries = color_primaries;
> -        st->codecpar->color_trc       = color_trc;
> -        st->codecpar->color_space     = color_matrix;
> -    } else if (!strncmp(color_parameter_type, "nclc", 4)) {
> -        /* color primaries, Table 4-4 */
> -        switch (color_primaries) {
> -        case 1: st->codecpar->color_primaries = AVCOL_PRI_BT709; break;
> -        case 5: st->codecpar->color_primaries = AVCOL_PRI_SMPTE170M;
> break;
> -        case 6: st->codecpar->color_primaries = AVCOL_PRI_SMPTE240M;
> break;
> -        }
> -        /* color transfer, Table 4-5 */
> -        switch (color_trc) {
> -        case 1: st->codecpar->color_trc = AVCOL_TRC_BT709; break;
> -        case 7: st->codecpar->color_trc = AVCOL_TRC_SMPTE240M; break;
> -        }
> -        /* color matrix, Table 4-6 */
> -        switch (color_matrix) {
> -        case 1: st->codecpar->color_space = AVCOL_SPC_BT709; break;
> -        case 6: st->codecpar->color_space = AVCOL_SPC_BT470BG; break;
> -        case 7: st->codecpar->color_space = AVCOL_SPC_SMPTE240M; break;
> -        }
>      }
> +    if (color_primaries >= AVCOL_PRI_NB)
> +        color_primaries = AVCOL_PRI_UNSPECIFIED;
> +    if (color_trc >= AVCOL_TRC_NB)
> +        color_trc = AVCOL_TRC_UNSPECIFIED;
> +    if (color_matrix >= AVCOL_SPC_NB)
> +        color_matrix = AVCOL_SPC_UNSPECIFIED;
> +    st->codecpar->color_primaries = color_primaries;
> +    st->codecpar->color_trc       = color_trc;
> +    st->codecpar->color_space     = color_matrix;
>      av_log(c->fc, AV_LOG_TRACE, "\n");
>
>      return 0;
> --
> 2.8.0.rc2
>
>

On Sat, Aug 20, 2016 at 04:15:20PM -0700, Steven Robertson wrote:
> Hey folks,
> 
> We anticipate professional video software will start producing files with
> the expanded codepoints Real Soon Now, and ProRes already supports Rec.
> 2020 and ST 2084 (see SMPTE RDD 36), so presumably MOV will get some of
> that soon too. (QuickTime in OS X El Cap already recognizes Rec. 2020 color
> in MOV, even though TN 2162 hasn't been updated yet.) Also it'd be nice to
> land this for the bugfix in SD.

> Anything I can do to help with the review?

yes
you can review someone elses patch see,
https://patchwork.ffmpeg.org/project/ffmpeg/list/
if you see one with wrong status there, you can create an account
and offer to help update the patches statuses
if every patch submitter reviewed one patch for each patch submitted
no one else would ever need to review a patch ;)
you could look at anything else on trac/wiki and help improve it
example:
https://trac.ffmpeg.org/wiki/SponsoringPrograms/Outreachy/2016-12
dates missing, not linked from frontpage of the wiki, ...
its publically editable just needs creating an account

ATM is just a bit busy time, not saying help would ever be unwelcome,
but GSoC end with lots of reviews, its august people are on vacation,
Outreachy needs admins & mentors, ive a heap of issues  should look
at and fix, ...

Thanks

[...]

On Wed, Aug 17, 2016 at 12:25:47AM -0700, Steven Robertson wrote:
> This change relaxes the whitelist on reading color metadata in MOV/BMFF
> containers. The whitelist on writing values is still in place.
> 
> As a consequence it also fixes an apparent bug in reading 'nclc' values.
> The 'nclc' spec [1] is in harmony with ISO 23001-8 for the values it
> lists, but the code getting removed was remapping 5->6 and 6->7 for
> primaries, which is incorrect, and was remapping 6->5 for color matrix
> ("colorspace" in the code), which is equivalent but an unnecessary
> inconsistency. This logic error doesn't appear in movenc.
> 
> Removing the whitelist allows proper conversion when the source codec
> relies on the container for proper signaling of newer codepoints, such
> as DNxHR and VP9. If converting to a codec or container that has updated
> its spec to include the new codepoints, the metadata will be preserved.
> If going back to MOV/BMFF, the output whitelist will still kick in, so
> this won't result in out-of-spec files being created.
> 
> [1] https://developer.apple.com/library/mac/technotes/tn2162/_index.html
> 
> Signed-off-by: Steven Robertson <steven@strobe.cc>
> ---
>  libavformat/mov.c | 40 +++++++++-------------------------------
>  1 file changed, 9 insertions(+), 31 deletions(-)

applied

thanks

[...]