| Message ID | 8C6F33C2-0A31-45D2-BE08-6EB0D341EA22@dericed.com |
|---|---|
| State | New |
| Headers | show |
Dave Rice wrote: >As required by Apple’s TN2162. >--- > libavformat/movenc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) LGTM
On Sun, Nov 19, 2017 at 10:05:56AM +0100, Reto Kromer wrote: > Dave Rice wrote: > > >As required by Apple’s TN2162. > >--- > > libavformat/movenc.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > LGTM will apply thanks [...]
On 11/18/2017 11:19 PM, Dave Rice wrote: > From 41da5e48f8788b85dd7a382030bb2866c506cc03 Mon Sep 17 00:00:00 2001 > From: Dave Rice <dave@dericed.com> > Date: Sat, 18 Nov 2017 20:31:27 -0500 > Subject: [PATCH 3/4] avformat/movenc: force colr atom for uncompressed yuv in > mov > MIME-Version: 1.0 > Content-Type: text/plain; charset=UTF-8 > Content-Transfer-Encoding: 8bit > > As required by Apple’s TN2162. > --- > libavformat/movenc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavformat/movenc.c b/libavformat/movenc.c > index aaa1dedfd7..86960b19c1 100644 > --- a/libavformat/movenc.c > +++ b/libavformat/movenc.c > @@ -1978,7 +1978,7 @@ static int mov_write_video_tag(AVIOContext *pb, MOVMuxContext *mov, MOVTrack *tr > 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 (mov->flags & FF_MOV_FLAG_WRITE_COLR || uncompressed_ycbcr) { > if (track->mode == MODE_MOV || track->mode == MODE_MP4) > mov_write_colr_tag(pb, track); > else > The write_colr option says "Write colr atom (Experimental, may be renamed or changed, do not use from scripts)". Does that still apply? Is the feature/spec still experimental? If not, then the option and flag could be removed as well as part of this patch.
On Sun, Nov 19, 2017 at 10:24:41PM -0300, James Almer wrote: > On 11/18/2017 11:19 PM, Dave Rice wrote: > > From 41da5e48f8788b85dd7a382030bb2866c506cc03 Mon Sep 17 00:00:00 2001 > > From: Dave Rice <dave@dericed.com> > > Date: Sat, 18 Nov 2017 20:31:27 -0500 > > Subject: [PATCH 3/4] avformat/movenc: force colr atom for uncompressed yuv in > > mov > > MIME-Version: 1.0 > > Content-Type: text/plain; charset=UTF-8 > > Content-Transfer-Encoding: 8bit > > > > As required by Apple’s TN2162. > > --- > > libavformat/movenc.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/libavformat/movenc.c b/libavformat/movenc.c > > index aaa1dedfd7..86960b19c1 100644 > > --- a/libavformat/movenc.c > > +++ b/libavformat/movenc.c > > @@ -1978,7 +1978,7 @@ static int mov_write_video_tag(AVIOContext *pb, MOVMuxContext *mov, MOVTrack *tr > > 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 (mov->flags & FF_MOV_FLAG_WRITE_COLR || uncompressed_ycbcr) { > > if (track->mode == MODE_MOV || track->mode == MODE_MP4) > > mov_write_colr_tag(pb, track); > > else > > > > The write_colr option says "Write colr atom (Experimental, may be > renamed or changed, do not use from scripts)". Does that still apply? Is > the feature/spec still experimental? > > If not, then the option and flag could be removed as well as part of > this patch. hmm, ill wait with applying this patch until this is decided thanks [...]
2017-11-20 2:24 GMT+01:00 James Almer <jamrial@gmail.com>: > On 11/18/2017 11:19 PM, Dave Rice wrote: >> From 41da5e48f8788b85dd7a382030bb2866c506cc03 Mon Sep 17 00:00:00 2001 >> From: Dave Rice <dave@dericed.com> >> Date: Sat, 18 Nov 2017 20:31:27 -0500 >> Subject: [PATCH 3/4] avformat/movenc: force colr atom for uncompressed yuv in >> mov >> MIME-Version: 1.0 >> Content-Type: text/plain; charset=UTF-8 >> Content-Transfer-Encoding: 8bit >> >> As required by Apple’s TN2162. >> --- >> libavformat/movenc.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavformat/movenc.c b/libavformat/movenc.c >> index aaa1dedfd7..86960b19c1 100644 >> --- a/libavformat/movenc.c >> +++ b/libavformat/movenc.c >> @@ -1978,7 +1978,7 @@ static int mov_write_video_tag(AVIOContext *pb, MOVMuxContext *mov, MOVTrack *tr >> 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 (mov->flags & FF_MOV_FLAG_WRITE_COLR || uncompressed_ycbcr) { >> if (track->mode == MODE_MOV || track->mode == MODE_MP4) >> mov_write_colr_tag(pb, track); >> else >> > > The write_colr option says "Write colr atom (Experimental, may be > renamed or changed, do not use from scripts)". Does that still apply? Is > the feature/spec still experimental? > > If not, then the option and flag could be removed as well as part of > this patch. I believe it should be removed in a follow-up patch. Carl Eugen
> On Nov 20, 2017, at 9:01 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > 2017-11-20 2:24 GMT+01:00 James Almer <jamrial@gmail.com <mailto:jamrial@gmail.com>>: >> On 11/18/2017 11:19 PM, Dave Rice wrote: >>> From 41da5e48f8788b85dd7a382030bb2866c506cc03 Mon Sep 17 00:00:00 2001 >>> From: Dave Rice <dave@dericed.com> >>> Date: Sat, 18 Nov 2017 20:31:27 -0500 >>> Subject: [PATCH 3/4] avformat/movenc: force colr atom for uncompressed yuv in >>> mov >>> MIME-Version: 1.0 >>> Content-Type: text/plain; charset=UTF-8 >>> Content-Transfer-Encoding: 8bit >>> >>> As required by Apple’s TN2162. >>> --- >>> libavformat/movenc.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/libavformat/movenc.c b/libavformat/movenc.c >>> index aaa1dedfd7..86960b19c1 100644 >>> --- a/libavformat/movenc.c >>> +++ b/libavformat/movenc.c >>> @@ -1978,7 +1978,7 @@ static int mov_write_video_tag(AVIOContext *pb, MOVMuxContext *mov, MOVTrack *tr >>> 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 (mov->flags & FF_MOV_FLAG_WRITE_COLR || uncompressed_ycbcr) { >>> if (track->mode == MODE_MOV || track->mode == MODE_MP4) >>> mov_write_colr_tag(pb, track); >>> else >>> >> >> The write_colr option says "Write colr atom (Experimental, may be >> renamed or changed, do not use from scripts)". Does that still apply? Is >> the feature/spec still experimental? >> >> If not, then the option and flag could be removed as well as part of >> this patch. > > I believe it should be removed in a follow-up patch. I am hesitant to see write_colr option removed since the guesswork used here https://github.com/FFmpeg/FFmpeg/blob/8f4702a93f87f9f76563e80f1ae2141a40029d9d/libavformat/movenc.c#L1747-L1775 would then be applied to all movs of standard HD and SD size and cause many ffmpeg outputs to appear differently after the change. Could the guesswork here be removed? Or could write_colr be enabled with the option removed while the guesswork itself has its own option? Dave Rice
On 11/20/2017 2:50 PM, Dave Rice wrote: > I am hesitant to see write_colr option removed since the guesswork used here https://github.com/FFmpeg/FFmpeg/blob/8f4702a93f87f9f76563e80f1ae2141a40029d9d/libavformat/movenc.c#L1747-L1775 would then be applied to all movs of standard HD and SD size and cause many ffmpeg outputs to appear differently after the change. 100% agree with this. We *should not* write guessed color information based off of resolution by default. Further, if no color information is present, no colr atom should be written at all, unless it is forced by the user (aka the write_colr option right now). > Could the guesswork here be removed? Or could write_colr be enabled with the option removed while the guesswork itself has its own option? If the quesswork is removed or put under an option, I still maintain no colr atom should be written if no color information is known. Doing so is just asking for trouble, in my opinion. - Derek
> On Nov 20, 2017, at 9:55 AM, Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote: > > On 11/20/2017 2:50 PM, Dave Rice wrote: >> I am hesitant to see write_colr option removed since the guesswork used here https://github.com/FFmpeg/FFmpeg/blob/8f4702a93f87f9f76563e80f1ae2141a40029d9d/libavformat/movenc.c#L1747-L1775 would then be applied to all movs of standard HD and SD size and cause many ffmpeg outputs to appear differently after the change. > > 100% agree with this. We *should not* write guessed color information based > off of resolution by default. > > Further, if no color information is present, no colr atom should be written > at all, unless it is forced by the user (aka the write_colr option right > now). TN2162 requires a colr atom for uncompressed yuv (including v210, v308, v408, etc) in mov, so I'd prefer to write it in this case. Note that the colr atom provides an option for unspecified for each of the color values, so there's a method to write a colr atom which basically says ¯\_(ツ)_/¯. >> Could the guesswork here be removed? Or could write_colr be enabled with the option removed while the guesswork itself has its own option? > > If the quesswork is removed or put under an option, I still maintain no > colr atom should be written if no color information is known. Doing > so is just asking for trouble, in my opinion. I disagree. I'd prefer to follow the specification and write a colr atom (in the case of uncompressed yuv in mov) that say the colr is unspecified rather than to write no colr atom at all and create an invalid file. See https://developer.apple.com/library/content/technotes/tn2162/_index.html#//apple_ref/doc/uid/DTS40013070-CH1-TNTAG9. I do agree that guesswork should be avoided or provided under an option that users could opt into. I suppose my preference order from support to oppose for writing uncompressed yuv in mov would be: 1: write a colr atom in all cases (if unknown, use unspecified values in colr atom) 2: write a colr atom if color data the known (no colr atom if unknown) 3: write a colr atom in all cases (if unknown, just make stuff up, #yolo) Dave Rice
On 11/20/2017 3:19 PM, Dave Rice wrote: > TN2162 requires a colr atom for uncompressed yuv (including v210, v308, v408, etc) in mov, so I'd prefer to write it in this case. Note that the colr atom provides an option for unspecified for each of the color values, so there's a method to write a colr atom which basically says ¯\_(ツ)_/¯. [...] > I disagree. I'd prefer to follow the specification and write a colr atom (in the case of uncompressed yuv in mov) that say the colr is unspecified rather than to write no colr atom at all and create an invalid file. See https://developer.apple.com/library/content/technotes/tn2162/_index.html#//apple_ref/doc/uid/DTS40013070-CH1-TNTAG9. > > I do agree that guesswork should be avoided or provided under an option that users could opt into. I suppose my preference order from support to oppose for writing uncompressed yuv in mov would be: > > 1: write a colr atom in all cases (if unknown, use unspecified values in colr atom) > 2: write a colr atom if color data the known (no colr atom if unknown) > 3: write a colr atom in all cases (if unknown, just make stuff up, #yolo) My opinion falls somewhere in between, to something like this (in pseudo-code): if (colors_known) { write_colr(vals); } else if (uncompressed_yuv) { if (guesswork_user_option) { write_colr(guessed_hacky_crap); } else { write_colr(unspecified); } } else if (guesswork_user_option) { write_colr(guessed_hacky_crap); } else { // Don't write a colr atom because it adds no value, to write unspecified in it // and no spec requires it for compressed streams. } Does this make sense? - Derek
> On Nov 20, 2017, at 11:22 AM, Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote: > > On 11/20/2017 3:19 PM, Dave Rice wrote: >> TN2162 requires a colr atom for uncompressed yuv (including v210, v308, v408, etc) in mov, so I'd prefer to write it in this case. Note that the colr atom provides an option for unspecified for each of the color values, so there's a method to write a colr atom which basically says ¯\_(ツ)_/¯. > > [...] > >> I disagree. I'd prefer to follow the specification and write a colr atom (in the case of uncompressed yuv in mov) that say the colr is unspecified rather than to write no colr atom at all and create an invalid file. See https://developer.apple.com/library/content/technotes/tn2162/_index.html#//apple_ref/doc/uid/DTS40013070-CH1-TNTAG9. >> >> I do agree that guesswork should be avoided or provided under an option that users could opt into. I suppose my preference order from support to oppose for writing uncompressed yuv in mov would be: >> >> 1: write a colr atom in all cases (if unknown, use unspecified values in colr atom) >> 2: write a colr atom if color data the known (no colr atom if unknown) >> 3: write a colr atom in all cases (if unknown, just make stuff up, #yolo) > > My opinion falls somewhere in between, to something like this (in pseudo-code): > > if (colors_known) { > write_colr(vals); > } else if (uncompressed_yuv) { > if (guesswork_user_option) { > write_colr(guessed_hacky_crap); > } else { > write_colr(unspecified); > } > } else if (guesswork_user_option) { > write_colr(guessed_hacky_crap); > } else { > // Don't write a colr atom because it adds no value, to write unspecified in it > // and no spec requires it for compressed streams. > } What do you propose as the default for guessed_hacky_crap? Also are there supporters for the need of a guessed_hacky_crap optio? Is there precedence in ffmpeg to enable/disable guesswork via a user option? Dave
On 11/20/2017 4:45 PM, Dave Rice wrote:
> What do you propose as the default for guessed_hacky_crap? Also are there supporters for the need of a guessed_hacky_crap optio? Is there precedence in ffmpeg to enable/disable guesswork via a user option?
I personally would never use the guesswork stuff, since I think it is a
terrible idea, so can't really propose a default or support it.
The guesswork stuff was added at the same time as colr support, by Michael,
in 7b6f4191763a5ffde02fa198101aabc3ddb5bf61, so maybe he has some opinion.
- Derek
The guess work comes from this list: https://developer.apple.com/library/content/technotes/tn2162/_index.html#//apple_ref/doc/uid/DTS40013070-CH1-TNTAG10-SAMPLE__COLR__SETTINGS and I agree is the source of a lot of chaos, but it is what several tools have done in the absence of any specified values. My usage of the feature is to write the atom when I need to by explicitly enabling it on the command line and I also explicitly set the metadata either from the input file, or by overriding on the command line. Kevin On Mon, Nov 20, 2017 at 4:52 PM, Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote: > On 11/20/2017 4:45 PM, Dave Rice wrote: >> What do you propose as the default for guessed_hacky_crap? Also are there supporters for the need of a guessed_hacky_crap optio? Is there precedence in ffmpeg to enable/disable guesswork via a user option? > > I personally would never use the guesswork stuff, since I think it is a > terrible idea, so can't really propose a default or support it. > > The guesswork stuff was added at the same time as colr support, by Michael, > in 7b6f4191763a5ffde02fa198101aabc3ddb5bf61, so maybe he has some opinion. > > - Derek > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On 11/22/2017 1:33 PM, Kevin Wheatley wrote: > The guess work comes from this list: > > https://developer.apple.com/library/content/technotes/tn2162/_index.html#//apple_ref/doc/uid/DTS40013070-CH1-TNTAG10-SAMPLE__COLR__SETTINGS It's a mistake to conflate resolutions with the standards listed there. It does not list any heuristics, just a set of standards and associated values. No info falls under "Anything Else", IMO. > and I agree is the source of a lot of chaos, but it is what several > tools have done in the absence of any specified values. "Other tools use heuristics too" is a bad rationale for creating such files by default. > My usage of the feature is to write the atom when I need to by > explicitly enabling it on the command line and I also explicitly set > the metadata either from the input file, or by overriding on the > command line. This use-case is preserved if we leave the heuristics as an option, which I'm totally OK with. - Derek
diff --git a/libavformat/movenc.c b/libavformat/movenc.c index aaa1dedfd7..86960b19c1 100644 --- a/libavformat/movenc.c +++ b/libavformat/movenc.c @@ -1978,7 +1978,7 @@ static int mov_write_video_tag(AVIOContext *pb, MOVMuxContext *mov, MOVTrack *tr 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 (mov->flags & FF_MOV_FLAG_WRITE_COLR || uncompressed_ycbcr) { if (track->mode == MODE_MOV || track->mode == MODE_MP4) mov_write_colr_tag(pb, track); else
From 41da5e48f8788b85dd7a382030bb2866c506cc03 Mon Sep 17 00:00:00 2001 From: Dave Rice <dave@dericed.com> Date: Sat, 18 Nov 2017 20:31:27 -0500 Subject: [PATCH 3/4] avformat/movenc: force colr atom for uncompressed yuv in mov MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As required by Apple’s TN2162. --- libavformat/movenc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)