[FFmpeg-devel] [PATCH] mov: write colr by default
Michael Niedermayer
michaelni at gmx.at
Thu Mar 5 10:32:29 CET 2015
On Thu, Mar 05, 2015 at 10:23:02AM +0100, Robert Krüger wrote:
> 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.
if you store the exact colorspace, this can end up using different
colorspaces when the resulting file is read later, that again can
lead to different codepathes being used and if now someone forgot some
bitexact related flag in a test it could cause diference between 32&64
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
When you are offended at any man's fault, turn to yourself and study your
own failings. Then you will forget your anger. -- Epictetus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150305/ec1a9ecb/attachment.asc>
More information about the ffmpeg-devel
mailing list