[FFmpeg-devel] [PATCH] mov: write colr by default

Robert Krüger krueger at lesspain.de
Thu Mar 5 10:23:02 CET 2015


On Wed, Mar 4, 2015 at 7:14 PM, Michael Niedermayer <michaelni at gmx.at>
wrote:

> On Wed, Mar 04, 2015 at 07:07:35PM +0100, Robert Krüger wrote:
> > On Wed, Mar 4, 2015 at 6:57 PM, Michael Niedermayer <michaelni at gmx.at>
> > wrote:
> >
> > > On Wed, Mar 04, 2015 at 06:19:19PM +0100, Robert Krüger wrote:
> > > > On Wed, Mar 4, 2015 at 5:34 PM, Michael Niedermayer <
> michaelni at gmx.at>
> > > > wrote:
> > > >
> > > > > On Wed, Mar 04, 2015 at 03:40:09PM +0100, Robert Krüger wrote:
> > > > > > On Wed, Mar 4, 2015 at 11:05 AM, Michael Niedermayer <
> > > michaelni at gmx.at>
> > > > > > wrote:
> > > > > >
> > > > > > > On Wed, Mar 04, 2015 at 10:22:26AM +0100, Robert Krüger wrote:
> > > > > > > > On Tue, Mar 3, 2015 at 9:27 PM, Michael Niedermayer <
> > > > > michaelni at gmx.at>
> > > > > > > > wrote:
> > > > > > > >
> > > > > > > > > On Tue, Mar 03, 2015 at 09:24:17PM +0100, Robert Krüger
> wrote:
> > > > > > > > > > On Tue, Mar 3, 2015 at 7:23 PM, Michael Niedermayer <
> > > > > > > michaelni at gmx.at>
> > > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > > On Tue, Mar 03, 2015 at 06:17:22PM +0100, Robert Krüger
> > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > > This is based on an earlier patch by Derek
> > > > > > > > > > >
> > > > > > > > > > > please mention this in the commit message
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > OK, I will change that
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > that never went in because it
> > > > > > > > > > > > was argumented earlier that api breakage is not
> > > acceptable.
> > > > > > > However,
> > > > > > > > > that
> > > > > > > > > > > > was more or less relaxed after Michael noted that the
> > > > > replaced
> > > > > > > flag
> > > > > > > > > had
> > > > > > > > > > > > never been part of a release and since a number of
> people
> > > > > seem to
> > > > > > > > > agree,
> > > > > > > > > > > > this is the better default, I am submitting this
> patch
> > > now,
> > > > > to
> > > > > > > have
> > > > > > > > > it in
> > > > > > > > > > > > before the upcoming release.
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > Let me know if that will be accepted and I will
> modify
> > > the
> > > > > > > respective
> > > > > > > > > > > fate
> > > > > > > > > > > > tests as well.
> > > > > > > > > > >
> > > > > > > > > > > have you tested the generated mov and mp4 files with
> some
> > > > > common
> > > > > > > > > > > software packages ?
> > > > > > > > > > >
> > > > > > > > > > > checking random files on my disk it seems more than
> half
> > > the
> > > > > mov
> > > > > > > > > > > files contain a colr atom but i found just a single mp4
> > > with a
> > > > > colr
> > > > > > > > > > > atom, so especially testing the compatibility of the
> mp4
> > > files
> > > > > > > would
> > > > > > > > > > > be optimal before this is changed
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > OK, I will do some tests with VLC, Quicktime, Final Cut,
> > > Final
> > > > > Cut X,
> > > > > > > > > > Premiere and After Effects and maybe something else I
> find.
> > > > > > > > >
> > > > > > > > > thanks
> > > > > > > > >
> > > > > > > > >
> > > > > > > > I tried an mp4 file with a colr atom with VLC, Quicktime,
> Final
> > > Cut
> > > > > X,
> > > > > > > > Compressor, Premiere, After Effects and Adobe Media Encoder.
> > > None of
> > > > > > > those
> > > > > > > > had any problems with the file and colors looked normal too.
> > > > > > >
> > > > > > > ok then please submit a patch that also updates fate
> > > > > > >
> > > > > > >
> > > > > > here it is.
> > > > >
> > > > > >  b/libavformat/movenc.c                            |    6 +--
> > > > > >  b/libavformat/movenc.h                            |    2 -
> > > > > >  b/libavformat/version.h                           |    4 +-
> > > > > >  b/tests/fate/vcodec.mak                           |    8 ++--
> > > > > >  b/tests/ref/lavf/mov                              |   16
> ++++----
> > > > > >  b/tests/ref/seek/lavf-mov                         |   44
> > > > > +++++++++++-----------
> > > > > >  b/tests/ref/vsynth/vsynth1-avui                   |    4 +-
> > > > > >  b/tests/ref/vsynth/vsynth1-dnxhd-1080i            |    8 ++--
> > > > > >  b/tests/ref/vsynth/vsynth1-dnxhd-1080i-nocolr     |    4 ++
> > > > > >  b/tests/ref/vsynth/vsynth1-mpeg4                  |    4 +-
> > > > > >  b/tests/ref/vsynth/vsynth1-prores                 |    4 +-
> > > > > >  b/tests/ref/vsynth/vsynth1-prores_ks              |    4 +-
> > > > > >  b/tests/ref/vsynth/vsynth1-qtrle                  |    4 +-
> > > > > >  b/tests/ref/vsynth/vsynth1-qtrlegray              |    4 +-
> > > > > >  b/tests/ref/vsynth/vsynth1-svq1                   |    4 +-
> > > > > >  b/tests/ref/vsynth/vsynth2-avui                   |    4 +-
> > > > > >  b/tests/ref/vsynth/vsynth2-dnxhd-1080i            |    8 ++--
> > > > > >  b/tests/ref/vsynth/vsynth2-dnxhd-1080i-nocolr     |    4 ++
> > > > > >  b/tests/ref/vsynth/vsynth2-mpeg4                  |    4 +-
> > > > > >  b/tests/ref/vsynth/vsynth2-prores                 |    4 +-
> > > > > >  b/tests/ref/vsynth/vsynth2-prores_ks              |    4 +-
> > > > > >  b/tests/ref/vsynth/vsynth2-qtrle                  |    4 +-
> > > > > >  b/tests/ref/vsynth/vsynth2-qtrlegray              |    4 +-
> > > > > >  b/tests/ref/vsynth/vsynth2-svq1                   |    4 +-
> > > > > >  b/tests/ref/vsynth/vsynth3-dnxhd-1080i-nocolr     |    4 ++
> > > > > >  b/tests/ref/vsynth/vsynth3-mpeg4                  |    4 +-
> > > > > >  b/tests/ref/vsynth/vsynth3-prores                 |    4 +-
> > > > > >  b/tests/ref/vsynth/vsynth3-prores_ks              |    4 +-
> > > > > >  b/tests/ref/vsynth/vsynth3-qtrle                  |    4 +-
> > > > > >  b/tests/ref/vsynth/vsynth3-svq1                   |    4 +-
> > > > > >  b/tests/ref/vsynth/vsynth_lena-dnxhd-1080i-nocolr |    4 ++
> > > > > >  tests/ref/vsynth/vsynth1-dnxhd-1080i-colr         |    4 --
> > > > > >  tests/ref/vsynth/vsynth2-dnxhd-1080i-colr         |    4 --
> > > > > >  tests/ref/vsynth/vsynth3-dnxhd-1080i-colr         |    4 --
> > > > > >  tests/ref/vsynth/vsynth_lena-dnxhd-1080i-colr     |    4 --
> > > > > >  35 files changed, 101 insertions(+), 103 deletions(-)
> > > > > > f6cafbe678f902fd7410b7dee69d9f6e04e591c1
> > > mov_write_colr_by_default.patch
> > > > > > From f836ea0dcb181417b38a75302c1c5ffdeaa0fb4d Mon Sep 17 00:00:00
> > > 2001
> > > > > > From: =?UTF-8?q?Robert=20Kr=C3=BCger?= <krueger at lesspain.de>
> > > > > > Date: Tue, 3 Mar 2015 18:11:54 +0100
> > > > > > Subject: [PATCH 1/2] mov: make writing colr the default (based
> on a
> > > > > patch by
> > > > > >  derek buitenhuis)
> > > > >
> > > > > this breaks fate, the test needing the lena sample are missing an
> > > > > update
> > > > >
> > > > >
> > > > Strange, I adjusted the lena sample tests and here make fate runs
> without
> > > > errors and I have no outgoing changes. Which test fails for you?
> > >
> > > i needed these:
> > >
> > >  tests/ref/vsynth/vsynth_lena-avui        |    4 ++--
> > >  tests/ref/vsynth/vsynth_lena-dnxhd-1080i |    8 ++++----
> > >  tests/ref/vsynth/vsynth_lena-mpeg4       |    4 ++--
> > >  tests/ref/vsynth/vsynth_lena-prores      |    4 ++--
> > >  tests/ref/vsynth/vsynth_lena-prores_ks   |    4 ++--
> > >  tests/ref/vsynth/vsynth_lena-qtrle       |    4 ++--
> > >  tests/ref/vsynth/vsynth_lena-qtrlegray   |    4 ++--
> > >  tests/ref/vsynth/vsynth_lena-svq1        |    4 ++--
> > >
> > >
> > >
> > > >
> > > >
> > > > > also once one fixes this the fate scores differ between 32bit and
> > > > > 64bit on linux
> > > > > --- tests/ref/vsynth/vsynth3-dnxhd-1080i-nocolr 2015-03-04
> > > > > 16:03:41.759112905 +0100
> > > > > +++ tests/data/fate/vsynth3-dnxhd-1080i-nocolr  2015-03-04
> > > > > 17:13:31.971201181 +0100
> > > > > @@ -1,4 +1,4 @@
> > > > >  aaed55622ffd609d6b6114ee3da7f585
> > > > > *tests/data/fate/vsynth3-dnxhd-1080i-nocolr.mov
> > > > >  3031911 tests/data/fate/vsynth3-dnxhd-1080i-nocolr.mov
> > > > > -382fc519604abb5d87071bdce013cef9
> > > > > *tests/data/fate/vsynth3-dnxhd-1080i-nocolr.out.rawvideo
> > > > > -stddev:    7.81 PSNR: 30.28 MAXDIFF:   61 bytes:    86700/
>  8670
> > > > > +cda7487f1c77f26df24668faa750257d
> > > > > *tests/data/fate/vsynth3-dnxhd-1080i-nocolr.out.rawvideo
> > > > > +stddev:    7.83 PSNR: 30.25 MAXDIFF:   61 bytes:    86700/
>  8670
> > > > >
> > > > > iam starting to have a somewhat ungood feeling with doing this
> change
> > > > > short before the release
> > > > >
> > > > >
> > > > I can understand that. What are the options? Make a release without
> the
> > > > write_colr flag or make it with the flag and later deprecate it in
> favour
> > > > of the no_colr flag?
> > >
> > > i guess we can just add the text "Experimental" or something similar
> > > to it and then later change it
> > > removial would break a fate test which uses that flag
> > >
> > >
> > OK, then I'll prepare a patch for after the release, although I cannot
> test
> > the linux 32/64 bit thing.
>
> you dont have a 64bit box ?
> i guess mingw32/64 would behave likewise
>
>
No, I do not have a Linux box/environment. Only Mac. And also, I have not
the slightest idea why something as high-level as changing/inverting the
logic for inclusion of an atom should behave differently depending on 32/64
bit.


More information about the ffmpeg-devel mailing list