| Message ID | EA4A7746-3F0A-4B90-A67C-A2337365B3F1@dericed.com |
|---|---|
| State | Superseded |
| Headers | show |
On 7/7/2017 3:20 AM, Dave Rice wrote: > Resolves https://trac.ffmpeg.org/ticket/6145 and writes a clap atom that is coincident with the frames width and height. > > > From 23d80d0d47829fed61e817b1e7c3f6d420c9ab5c Mon Sep 17 00:00:00 2001 > From: Dave Rice <dave@dericed.com> > Date: Thu, 6 Jul 2017 21:12:38 -0400 > Subject: [PATCH] movenc: write clap tag > > --- > libavformat/movenc.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) Whoever pushes, add the ticket number into the commit message. [...] > + avio_wb32(pb, 0); /* horizOff_N (= 0) */ > + avio_wb32(pb, 1); /* horizOff_D (= 1) */ > + avio_wb32(pb, 0); /* vertOff_N (= 0) */ > + avio_wb32(pb, 1); /* vertOff_D (= 1) */ Near as I can tell, these related the active image area? Is it useful to ever set this to anything but 0 (aka the whole image is active)? Was thinking of stuff like analogue video, a la http://chromashift.org/aspectratio/. I assume the answer is probably 'no'. Patch itself LGTM. - Derek
On 7/7/2017 4:32 PM, Derek Buitenhuis wrote:
> Patch itself LGTM.
Just realized, this will change a bunch of FATE tests, won't it?
- Derek
On 7/7/2017 12:32 PM, Derek Buitenhuis wrote: > On 7/7/2017 3:20 AM, Dave Rice wrote: >> Resolves https://trac.ffmpeg.org/ticket/6145 and writes a clap atom that is coincident with the frames width and height. >> >> >> From 23d80d0d47829fed61e817b1e7c3f6d420c9ab5c Mon Sep 17 00:00:00 2001 >> From: Dave Rice <dave@dericed.com> >> Date: Thu, 6 Jul 2017 21:12:38 -0400 >> Subject: [PATCH] movenc: write clap tag >> >> --- >> libavformat/movenc.c | 19 +++++++++++++++++++ >> 1 file changed, 19 insertions(+) > > Whoever pushes, add the ticket number into the commit message. > > [...] > >> + avio_wb32(pb, 0); /* horizOff_N (= 0) */ >> + avio_wb32(pb, 1); /* horizOff_D (= 1) */ >> + avio_wb32(pb, 0); /* vertOff_N (= 0) */ >> + avio_wb32(pb, 1); /* vertOff_D (= 1) */ > > Near as I can tell, these related the active image area? Is it useful to ever set > this to anything but 0 (aka the whole image is active)? Was thinking of stuff like > analogue video, a la http://chromashift.org/aspectratio/. I assume the answer is > probably 'no'. > > Patch itself LGTM. Isn't this necessary only for files with raw video? As is, this box would be written on any mov file with a video stream.
On 7/7/2017 10:13 PM, James Almer wrote: > Isn't this necessary only for files with raw video? As is, this box > would be written on any mov file with a video stream. This was addressed a previous email: http://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-July/213350.html I guess the spec is up for interpretation. - Derek
> On Jul 7, 2017, at 11:32 AM, Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote: > > On 7/7/2017 3:20 AM, Dave Rice wrote: >> Resolves https://trac.ffmpeg.org/ticket/6145 and writes a clap atom that is coincident with the frames width and height. >> >> >> From 23d80d0d47829fed61e817b1e7c3f6d420c9ab5c Mon Sep 17 00:00:00 2001 >> From: Dave Rice <dave@dericed.com> >> Date: Thu, 6 Jul 2017 21:12:38 -0400 >> Subject: [PATCH] movenc: write clap tag >> >> --- >> libavformat/movenc.c | 19 +++++++++++++++++++ >> 1 file changed, 19 insertions(+) > > Whoever pushes, add the ticket number into the commit message. > > [...] > >> + avio_wb32(pb, 0); /* horizOff_N (= 0) */ >> + avio_wb32(pb, 1); /* horizOff_D (= 1) */ >> + avio_wb32(pb, 0); /* vertOff_N (= 0) */ >> + avio_wb32(pb, 1); /* vertOff_D (= 1) */ > > Near as I can tell, these related the active image area? Is it useful to ever set > this to anything but 0 (aka the whole image is active)? Usually not a good idea to set a denominator to zero. The clap atom documentation at https://developer.apple.com/library/content/documentation/QuickTime/QTFF/QTFFChap3/qtff3.html#//apple_ref/doc/uid/TP40000939-CH205-125850 <https://developer.apple.com/library/content/documentation/QuickTime/QTFF/QTFFChap3/qtff3.html#//apple_ref/doc/uid/TP40000939-CH205-125850> specifies zero for numerators and one for denominators as default when the ratio of an actual aperture is not used. > Was thinking of stuff like > analogue video, a la http://chromashift.org/aspectratio/. I assume the answer is > probably 'no’. The atom is analogous to the PixelCrop* elements of Matroska. For instance a standard definition NTSC video may typically be digitized to a frame size of 720x486 but the active image might only be 704x480, so the aperture atom could express the difference. > Patch itself LGTM. […] Thanks, Dave Rice
> On Jul 7, 2017, at 7:06 PM, Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote: > > On 7/7/2017 10:13 PM, James Almer wrote: >> Isn't this necessary only for files with raw video? As is, this box >> would be written on any mov file with a video stream. > > This was addressed a previous email: > > http://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-July/213350.html > > I guess the spec is up for interpretation. The quicktime spec says "This is a mandatory extension for all uncompressed Y´CbCr data formats”. It doesn’t clarify if the clap atom is recommended or not in mov files that are not “uncompressed Y´CbCr”, though it would make sense if the video container needs to store cropping data. I think constraining the change for only “uncompressed Y´CbCr” would be more cautious though. I’ll revise my patch to include the condition and resubmit. If the patch only impacts “uncompressed Y´CbCr” would any fate updates be needed? Dave Rice
diff --git a/libavformat/movenc.c b/libavformat/movenc.c index 88f2f2c819..31a5de8d5c 100644 --- a/libavformat/movenc.c +++ b/libavformat/movenc.c @@ -1669,6 +1669,21 @@ static int mov_write_sv3d_tag(AVFormatContext *s, AVIOContext *pb, AVSphericalMa return update_size(pb, sv3d_pos); } +static int mov_write_clap_tag(AVIOContext *pb, MOVTrack *track) +{ + avio_wb32(pb, 40); + ffio_wfourcc(pb, "clap"); + avio_wb32(pb, track->par->width); /* apertureWidth_N */ + avio_wb32(pb, 1); /* apertureWidth_D (= 1) */ + avio_wb32(pb, track->height); /* apertureHeight_N */ + avio_wb32(pb, 1); /* apertureHeight_D (= 1) */ + avio_wb32(pb, 0); /* horizOff_N (= 0) */ + avio_wb32(pb, 1); /* horizOff_D (= 1) */ + avio_wb32(pb, 0); /* vertOff_N (= 0) */ + avio_wb32(pb, 1); /* vertOff_D (= 1) */ + return 40; +} + static int mov_write_pasp_tag(AVIOContext *pb, MOVTrack *track) { AVRational sar; @@ -1939,6 +1954,10 @@ static int mov_write_video_tag(AVIOContext *pb, MOVMuxContext *mov, MOVTrack *tr av_log(mov->fc, AV_LOG_WARNING, "Not writing 'colr' atom. Format is not MOV or MP4.\n"); } + if (track->mode == MODE_MOV) { + mov_write_clap_tag(pb, track); + } + if (track->mode == MODE_MP4 && mov->fc->strict_std_compliance <= FF_COMPLIANCE_UNOFFICIAL) { AVStereo3D* stereo_3d = (AVStereo3D*) av_stream_get_side_data(track->st, AV_PKT_DATA_STEREO3D, NULL); AVSphericalMapping* spherical_mapping = (AVSphericalMapping*)av_stream_get_side_data(track->st, AV_PKT_DATA_SPHERICAL, NULL);
Resolves https://trac.ffmpeg.org/ticket/6145 and writes a clap atom that is coincident with the frames width and height. From 23d80d0d47829fed61e817b1e7c3f6d420c9ab5c Mon Sep 17 00:00:00 2001 From: Dave Rice <dave@dericed.com> Date: Thu, 6 Jul 2017 21:12:38 -0400 Subject: [PATCH] movenc: write clap tag --- libavformat/movenc.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)