Re: [FFmpeg-devel] [PATCH 2/2] Require compilers to support C17.
On Fri, Feb 9, 2024 at 11:22 AM Dominik 'Rathann' Mierzejewski wrote: > Even so, a C17-supporting compiler (gcc 11.2.1) is available for CentOS 7 > in the devtoolset-11-gcc package (from > http://mirror.centos.org/centos/7/sclo/x86_64/rh/). As a 'User' of the FFmpeg project, we have a lot of CentOS/RHEL 7/etc based headless machines which run FFmpeg and thus we compile versions of FFmpeg on this platform. This is currently quite common across the VFX industry as most of the industry is still in the process of moving away from this to something more recent see https://vfxplatform.com/. The availability of "newer" compilers via the devtool set should make this proposed requirement a relatively small obstacle for building newer versions. As such I think that communicating the proposed change and its implications in the next release versions and the FFmpeg website should be sufficient, if similar notes for other common platforms could be added to the release notes/changelog then at least for users of these Fedora/RHEL derivatives should be fine. I'm assuming anybody able to compile a custom FFmpeg build, is also able to arrange to install a prebuilt compiler, so as long as the dependency doesn't bleed through to a runtime one, all should be good. On the topic of why users may be so far behind the "current" for the OS is down to several factors, in VFX and animation it is not uncommon to work upon a project for many years prior to releasing, this means we tend to lock down software versions in use for long periods of time (switching out compilers, libraries etc can all change the results of computation and thus our images). Running multiple projects at once can make changing the OS a long duration process. We've recently seen a similar issue with vscode bumping glibc dependencies https://github.com/microsoft/vscode/issues/203375 though switching glibc versions is a lot more awkward than a compiler requirement. Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] movenc: add movie_timescale option instead of hardcoding 1000
This is quite similar to my patch suggestion from last year: http://ffmpeg.org/pipermail/ffmpeg-devel/2020-April/261088.html mine was missing some docs updates, and I followed the existing naming scheme of prefixing the argument as 'mov_' rather than movie, but otherwise is quite similar Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] libavutil/cpu: Fix definition of _GNU_SOURCE so it occurs before other includes
On Mon, Apr 12, 2021 at 12:22 PM wrote: > > From: Kevin Wheatley > > This fix moves the potential definition of _GNU_SOURCE prior to > any includes of system header files as required by the documentation > https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html > > This corrects the CPU_COUNT macro availability, resulting in > sched_getaffinity() being called on Linux systems. This then correctly > returns the number of CPUs when run under containers and other cases > where processor affinity has been setup prior to running FFmpeg > bump As an FYI the issue is triggered because the inclusion of the other system headers prior to the _GNU_SOURCE definition results in the CPU_COUNT macro not being defined, and so the affinity based CPU counting fails to be used and so the cpu count is incorrectly returned. Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] avfilter/vf_scale: set RGB to always be full range
On Wed, Sep 16, 2020 at 9:43 PM Jan Ekström wrote: > > On Wed, Sep 16, 2020 at 11:38 PM Michael Niedermayer > wrote: > > Does anything document the meaning of range or AVCOL_RANGE for RGB ? > > The enum is documented as "MPEG vs JPEG YUV range." > > > > What is limited range RGB ? what would the ranges of R,G,B be and where > > is that specified ? EBU only describes full range for 'file' based images https://tech.ebu.ch/docs/r/r103.pdf but ITU https://www.itu.int/dms_pubrec/itu-r/rec/bt/R-REC-BT.2100-2-201807-I!!PDF-E.pdf does describe narrow range RGB on p9 Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 2/3] avformat/movenc: Use base container timescale, instead of hard coded default
Signed-off-by: Kevin Wheatley --- libavformat/movenc.c | 22 +++--- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/libavformat/movenc.c b/libavformat/movenc.c index 7d79eca..508fa73 100644 --- a/libavformat/movenc.c +++ b/libavformat/movenc.c @@ -2879,7 +2879,7 @@ static int mov_write_tkhd_tag(AVIOContext *pb, MOVMuxContext *mov, MOVTrack *track, AVStream *st) { int64_t duration = av_rescale_rnd(calc_pts_duration(mov, track), - MOV_TIMESCALE, track->timescale, + mov->mov_timescale, track->timescale, AV_ROUND_UP); int version = duration < INT32_MAX ? 0 : 1; int flags = MOV_TKHD_FLAG_IN_MOVIE; @@ -3027,7 +3027,7 @@ static int mov_write_edts_tag(AVIOContext *pb, MOVMuxContext *mov, MOVTrack *track) { int64_t duration = av_rescale_rnd(calc_pts_duration(mov, track), - MOV_TIMESCALE, track->timescale, + mov->mov_timescale, track->timescale, AV_ROUND_UP); int version = duration < INT32_MAX ? 0 : 1; int entry_size, entry_count, size; @@ -3046,7 +3046,7 @@ static int mov_write_edts_tag(AVIOContext *pb, MOVMuxContext *mov, } } -delay = av_rescale_rnd(start_dts + start_ct, MOV_TIMESCALE, +delay = av_rescale_rnd(start_dts + start_ct, mov->mov_timescale, track->timescale, AV_ROUND_DOWN); version |= delay < INT32_MAX ? 0 : 1; @@ -3081,8 +3081,8 @@ static int mov_write_edts_tag(AVIOContext *pb, MOVMuxContext *mov, /* Avoid accidentally ending up with start_ct = -1 which has got a * special meaning. Normally start_ct should end up positive or zero * here, but use FFMIN in case dts is a small positive integer - * rounded to 0 when represented in MOV_TIMESCALE units. */ -av_assert0(av_rescale_rnd(start_dts, MOV_TIMESCALE, track->timescale, AV_ROUND_DOWN) <= 0); + * rounded to 0 when represented in mov->mov_timescale units. */ +av_assert0(av_rescale_rnd(start_dts, mov->mov_timescale, track->timescale, AV_ROUND_DOWN) <= 0); start_ct = -FFMIN(start_dts, 0); /* Note, this delay is calculated from the pts of the first sample, * ensuring that we don't reduce the duration for cases with @@ -3316,7 +3316,7 @@ static int mov_write_mvhd_tag(AVIOContext *pb, MOVMuxContext *mov) if (mov->tracks[i].entry > 0 && mov->tracks[i].timescale) { int64_t max_track_len_temp = av_rescale_rnd( calc_pts_duration(mov, &mov->tracks[i]), -MOV_TIMESCALE, +mov->mov_timescale, mov->tracks[i].timescale, AV_ROUND_UP); if (max_track_len < max_track_len_temp) @@ -3345,7 +3345,7 @@ static int mov_write_mvhd_tag(AVIOContext *pb, MOVMuxContext *mov) avio_wb32(pb, mov->time); /* creation time */ avio_wb32(pb, mov->time); /* modification time */ } -avio_wb32(pb, MOV_TIMESCALE); +avio_wb32(pb, mov->mov_timescale); (version == 1) ? avio_wb64(pb, max_track_len) : avio_wb32(pb, max_track_len); /* duration of longest track */ avio_wb32(pb, 0x0001); /* reserved (preferred rate) 1.0 = normal */ @@ -5921,7 +5921,7 @@ static int mov_create_chapter_track(AVFormatContext *s, int tracknum) track->mode = mov->mode; track->tag = MKTAG('t','e','x','t'); -track->timescale = MOV_TIMESCALE; +track->timescale = mov->mov_timescale; track->par = avcodec_parameters_alloc(); if (!track->par) return AVERROR(ENOMEM); @@ -5982,8 +5982,8 @@ static int mov_create_chapter_track(AVFormatContext *s, int tracknum) AVChapter *c = s->chapters[i]; AVDictionaryEntry *t; -int64_t end = av_rescale_q(c->end, c->time_base, (AVRational){1,MOV_TIMESCALE}); -pkt.pts = pkt.dts = av_rescale_q(c->start, c->time_base, (AVRational){1,MOV_TIMESCALE}); +int64_t end = av_rescale_q(c->end, c->time_base, (AVRational){1,mov->mov_timescale}); +pkt.pts = pkt.dts = av_rescale_q(c->start, c->time_base, (AVRational){1,mov->mov_timescale}); pkt.duration = end - pkt.dts; if ((t = av_dict_get(c->metadata, "title", NULL, 0))) { @@ -6518,7 +6518,7 @@ static int mov_init(AVFormatContext *s) } else if (st->codecpar->codec_type == AVMEDIA_TYPE_DATA) {
[FFmpeg-devel] [PATCH 3/3] avformat/movenc: Add an automatic timescale computation
Activated when the -mov_timescale command line/MOVMuxContext parameter is set to 0 or less. If the user passes any value greater than 0, then it will be used as-is. The default value is kept unchanged at 1000. When active, it uses the base assumption from the QuickTime specification of 600 and combines the video stream time bases to compute an accurate answer if possible. In cases when the first stream result falls outside MOV_MAX_AUTO_TIMESCALE or if a non-integer video stream is encounted, then the first stream's time_base will be used as the base. Signed-off-by: Kevin Wheatley --- libavformat/movenc.c | 71 libavformat/movenc.h | 1 + 2 files changed, 72 insertions(+) diff --git a/libavformat/movenc.c b/libavformat/movenc.c index 508fa73..8edb848 100644 --- a/libavformat/movenc.c +++ b/libavformat/movenc.c @@ -6208,6 +6208,70 @@ static int mov_create_dvd_sub_decoder_specific_info(MOVTrack *track, return 0; } +static int mov_determine_movie_timescale(AVFormatContext *s, MOVMuxContext *mov) +{ +// Assume typical integer frame rates: +// 600 is a common multiple of 24, 25, 30, 50, 60, etc. +// see p221, https://developer.apple.com/standards/qtff-2001.pdf +int timescale = 600; +AVRational temp; +int exact; +int first_video_track = 1; + +// If the user specified a timescale, just use it as-is +if (mov->mov_timescale > 0) { +return mov->mov_timescale; +} + +// Determine the timescale, based on the video stream time_base values +for (int i = 0; i < s->nb_streams; i++) { +AVStream *st = s->streams[i]; +if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO) { + +// If the first video track is not an integer, use its denominator +// as the basis of the remaining calculations +if (first_video_track && st->time_base.num != 1) { +timescale = st->time_base.den; +av_log(s, AV_LOG_VERBOSE, "Using first video stream non-integer time_base: %d/%d\n", + st->time_base.den, st->time_base.num); +} + +// determine if we can make an even multiple of the current timescale and the video track +av_log(s, AV_LOG_VERBOSE, "Current estimated timescale: %d\n", timescale); +av_log(s, AV_LOG_TRACE, "Stream #%d time_base: %d/%d\n", i, + st->time_base.num, st->time_base.den); + +// Try keep the time_scale within a sensible bound +exact = av_reduce(&temp.num, &temp.den, + (int64_t)timescale * st->time_base.num, + st->time_base.den, + MOV_MAX_AUTO_TIMESCALE); + +// Use a scaled version of the timescale +if (exact) { +timescale *= temp.den; +av_log(s, AV_LOG_TRACE, "Adjusted timescale: %d/%d %d\n", + temp.den, temp.num, timescale); +} else { +// We overflowed try the denominator as is +timescale = temp.den; +av_log(s, AV_LOG_WARNING, "Timescale calculation out of bounds approximating %d/%d %d\n", + temp.den, temp.num, timescale); +} + +if (first_video_track && ((timescale > MOV_MAX_AUTO_TIMESCALE) || !exact)) { +timescale = st->time_base.den; +av_log(s, AV_LOG_WARNING, "Potentially large timescale, " + "using first video stream time_base: %d/%d\n", + st->time_base.den, st->time_base.num); +} +first_video_track = 0; +} +} +av_log(s, AV_LOG_VERBOSE, "Final timescale: %d\n", timescale); +return timescale; +} + static int mov_init(AVFormatContext *s) { MOVMuxContext *mov = s->priv_data; @@ -6266,6 +6330,13 @@ static int mov_init(AVFormatContext *s) mov->reserved_moov_size = -1; } +mov->mov_timescale = mov_determine_movie_timescale(s, mov); +if (mov->mov_timescale > MOV_MAX_AUTO_TIMESCALE) { +av_log(s, AV_LOG_WARNING, "Potentially large timescale %d, use " + "-mov_timescale if you have issues\n", + mov->mov_timescale); +} + if (mov->use_editlist < 0) { mov->use_editlist = 1; if (mov->flags & FF_MOV_FLAG_FRAGMENT && diff --git a/libavformat/movenc.h b/libavformat/movenc.h index 322968c..4d6b7b7 100644 --- a/libavformat/movenc.h +++ b/libavformat/movenc.h @@ -30,6 +30,7 @@ #define MOV_FRAG_INFO_ALLOC_INCREMENT 64 #define MOV_INDEX_CLUSTER_SIZE 1024 #define MOV_TIMESC
[FFmpeg-devel] [PATCH v3 0/3] avformat/movenc: Support for variable timescale in mov containers
v3 - rebased to current master This set of patches work together to facilitate user specified timescales for the Movie Header Atom 'mvhd', which allows constant sample table durations for all frame rates. Currently there is a fixed timescale of 1000, which is not an even multiple of all the typical video frame/field rates. This means when performing certain duration based operations it is possible to be inaccurate. The default behaviour is left at the current default defined by MOV_TIMESCALE, but can be over ridden by using the -mov_timescale option. Typical values of 600 would work for 24, 25 and 30 FPS, for 23.976 and other fractional rates could use 2997 same as Avid (or 24000 though caution should be used when encoding long durations). An automatic mode has been added if a negative value is passed to the option. This will compute an appropriate time scale starting from the lowest numbered video stream's time scale. Example usage that has better behaviour than the current: # Encode 50 frames at 24FPS and concatenate 5 copies ffmpeg -f lavfi -i smptebars=duration=2.08:size=1920x1080:rate=24 \ -codec dnxhd -pix_fmt yuv422p -b:v 115M smptebars_dnx_1000.mov cat < concat_1000.txt file smptebars_dnx_1000.mov file smptebars_dnx_1000.mov file smptebars_dnx_1000.mov file smptebars_dnx_1000.mov file smptebars_dnx_1000.mov EOF ffmpeg -f concat -i concat_1000.txt -c copy smpte_concat_1000.mov ffprobe smpte_concat_1000.mov The output of ffprobe will show a frame rate of 23.99 due to the effect of ther sample durations in the stts entries. With the new option of -mov_timescale set to 600: ffmpeg -f lavfi -i smptebars=duration=2.08:size=1920x1080:rate=24 \ -codec dnxhd -pix_fmt yuv422p -b:v 115M -mov_timescale 600 smptebars_dnx_600.mov cat < concat_600.txt file smptebars_dnx_600.mov file smptebars_dnx_600.mov file smptebars_dnx_600.mov file smptebars_dnx_600.mov file smptebars_dnx_600.mov EOF ffmpeg -f concat -i concat_600.txt -c copy -mov_timescale 600 smpte_concat_600.mov ffprobe smpte_concat_600.mov The durations all line up, the stts table is smaller and no rounding issues occur. Kevin Kevin Wheatley (3): avformat/movenc: Add command line option to set base mov file timescale avformat/movenc: Use base container timescale, instead of hard coded default avformat/movenc: Add an automatic timescale computation libavformat/movenc.c | 94 ++-- libavformat/movenc.h | 2 ++ 2 files changed, 85 insertions(+), 11 deletions(-) -- 1.8.5.6 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 1/3] avformat/movenc: Add command line option to set base mov file timescale
Signed-off-by: Kevin Wheatley --- libavformat/movenc.c | 1 + libavformat/movenc.h | 1 + 2 files changed, 2 insertions(+) diff --git a/libavformat/movenc.c b/libavformat/movenc.c index 253cff8..7d79eca 100644 --- a/libavformat/movenc.c +++ b/libavformat/movenc.c @@ -91,6 +91,7 @@ static const AVOption options[] = { { "min_frag_duration", "Minimum fragment duration", offsetof(MOVMuxContext, min_fragment_duration), AV_OPT_TYPE_INT, {.i64 = 0}, 0, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM}, { "frag_size", "Maximum fragment size", offsetof(MOVMuxContext, max_fragment_size), AV_OPT_TYPE_INT, {.i64 = 0}, 0, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM}, { "ism_lookahead", "Number of lookahead entries for ISM files", offsetof(MOVMuxContext, ism_lookahead), AV_OPT_TYPE_INT, {.i64 = 0}, 0, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM}, +{ "mov_timescale", "set timescale of mov container", offsetof(MOVMuxContext, mov_timescale), AV_OPT_TYPE_INT, {.i64 = MOV_TIMESCALE}, 0, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM}, { "video_track_timescale", "set timescale of all video tracks", offsetof(MOVMuxContext, video_track_timescale), AV_OPT_TYPE_INT, {.i64 = 0}, 0, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM}, { "brand","Override major brand", offsetof(MOVMuxContext, major_brand), AV_OPT_TYPE_STRING, {.str = NULL}, .flags = AV_OPT_FLAG_ENCODING_PARAM }, { "use_editlist", "use edit list", offsetof(MOVMuxContext, use_editlist), AV_OPT_TYPE_BOOL, {.i64 = -1}, -1, 1, AV_OPT_FLAG_ENCODING_PARAM}, diff --git a/libavformat/movenc.h b/libavformat/movenc.h index 997b2d6..322968c 100644 --- a/libavformat/movenc.h +++ b/libavformat/movenc.h @@ -205,6 +205,7 @@ typedef struct MOVMuxContext { AVIOContext *mdat_buf; int first_trun; +int mov_timescale; int video_track_timescale; int reserved_moov_size; ///< 0 for disabled, -1 for automatic, size otherwise -- 1.8.5.6 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] avcodec/exr: add cineon lin2log trc
On Fri, Mar 6, 2020 at 10:19 AM Hendrik Leppkes wrote: > AVColorTransferCharacteristic should follow ISO/IEC 23001-8 and its > following standards (ISO/IEC 23091 I believe). Not sure we have a > solution for specialized variants, but adding one right there would > collide with the next addition to the standard... Agreed, A publicly "aligned twin text" version is available as ITU-T H.273 https://www.itu.int/rec/T-REC-H.273-201612-I/en shows that value is 'reserved'. I would also suggest that there are a few implementations of the Cineon log to lin conversion and as such I'd reject it as is. You will need to pass in the parameters for the various variables white, black etc. You will also need to add a parameter for the density per code value constant (0.002 by default convention). Not all implementations use a gain scaling, though strictly these are no longer "Cineon" per se, but you will end up with needing various camera vendor equivalents. This would result in a large proliferation of equations, which I don't believe are core to the FFmpeg code. You would perhaps be better creating a specialist filter and implementing it using OCIO (as suggested https://github.com/AcademySoftwareFoundation/tac/tree/master/gsoc), or extending the current 3D LUT code to read Cinespace or other 3D LUT formats that support a pre-shaper and handle the float->integer conversion that way. Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH v2 0/3] avformat/movenc: Support for variable timescale in mov containers
This set of patches work together to facilitate user specified timescales for the Movie Header Atom 'mvhd', which allows constant sample table durations for all frame rates. Currently there is a fixed timescale of 1000, which is not an even multiple of all the typical video frame/field rates. This means when performing certain duration based operations it is possible to be inaccurate. The default behaviour is left at the current default defined by MOV_TIMESCALE, but can be over ridden by using the -mov_timescale option. Typical values of 600 would work for 24, 25 and 30 FPS, for 23.976 and other fractional rates could use 2997 same as Avid (or 24000 though caution should be used when encoding long durations). Example usage that has better behaviour than the current: # Encode 50 frames at 24FPS and concatenate 5 copies ffmpeg -f lavfi -i smptebars=duration=2.08:size=1920x1080:rate=24 \ -codec dnxhd -pix_fmt yuv422p -b:v 115M smptebars_dnx_1000.mov cat < concat_1000.txt file smptebars_dnx_1000.mov file smptebars_dnx_1000.mov file smptebars_dnx_1000.mov file smptebars_dnx_1000.mov file smptebars_dnx_1000.mov EOF ffmpeg -f concat -i concat_1000.txt -c copy smpte_concat_1000.mov ffprobe smpte_concat_1000.mov The output of ffprobe will show a frame rate of 23.99 due to the effect of ther sample durations in the stts entries. With the new option of -mov_timescale set to 600: ffmpeg -f lavfi -i smptebars=duration=2.08:size=1920x1080:rate=24 \ -codec dnxhd -pix_fmt yuv422p -b:v 115M -mov_timescale 600 smptebars_dnx_600.mov cat < concat_600.txt file smptebars_dnx_600.mov file smptebars_dnx_600.mov file smptebars_dnx_600.mov file smptebars_dnx_600.mov file smptebars_dnx_600.mov EOF ffmpeg -f concat -i concat_600.txt -c copy -mov_timescale 600 smpte_concat_600.mov ffprobe smpte_concat_600.mov The durations all line up, the stts table is smaller and no rounding issues occur. Kevin Kevin Wheatley (3): avformat/movenc: Add command line option to set base mov file timescale avformat/movenc: Use base container timescale, instead of hard coded default avformat/movenc: Add an automatic timescale computation libavformat/movenc.c | 94 ++-- libavformat/movenc.h | 2 ++ 2 files changed, 85 insertions(+), 11 deletions(-) v2-0001-avformat-movenc-Add-command-line-option-to-set-ba.patch Description: Binary data v2-0002-avformat-movenc-Use-base-container-timescale-inst.patch Description: Binary data v2-0003-avformat-movenc-Add-an-automatic-timescale-comput.patch Description: Binary data ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] avformat/movenc: Support for variable timescale in mov containers
On Tue, Nov 5, 2019 at 4:52 PM Michael Niedermayer wrote: > Assuming this doesnt violate any specifications and assuming that > it works with all players. > Then it would make sense to select this value automatically based > on the stream timestamps or timebases. > maybe there could be still a -mov_timescale but with an option for > "auto" to autoselect a small one which allows accurate representation > of most streams Michael, thanks for the feedback, I did consider this, but I am not that familiar with the code base of FFmpeg to know what the 'right' data fields to use are and at what point during the initialisation process of the movenc code it is valid to use them. There certainly seams to be a number of points at which the code tries to compute a lowest common multiple in the general FFmpeg code and a number of fields representing timebases, frame rates etc. If somebody can help point me at what I should use, then I'm happy to add something to automatically determine something. In terms of the specification, the Apple documentation certainly recommends the use of 600 on p221 of https://developer.apple.com/standards/qtff-2001.pdf for most integer frame rates, but as the following section in that document says this does not work for 23.976 or 29.97. For newer frame rates like 48 and any other number of 'new' content types, those number fail so whilst I'd suggest using the Apple number for the well used rates, I guess a computation would be needed for other things. Finally what about the default behaviour, would it be the old use a fixed setting or would we change the default to be automatic with the option of explicitly passing in 1000 to mimic the previous behaviour if required (plus any updates to fate to account for the change). Thanks Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH] avformat/movenc: Support for variable timescale in mov containers
This pair of patches work together to facilitate user specified timescales for the Movie Header Atom 'mvhd', which allows constant sample table durations for all frame rates. Currently there is a fixed timescale of 1000, which is not an even multiple of all the typical video frame/field rates. This means when performing certain duration based operations it is possible to be inaccurate. The default behaviour is left at the current default defined by MOV_TIMESCALE, but can be over ridden by using the -mov_timescale option. Typical values of 600 would work for 24, 25 and 30 FPS, for 23.976 and other fractional rates could use 2997 same as Avid (or 24000 though caution should be used when encoding long durations). Example usage that has better behaviour than the current: # Encode 50 frames at 24FPS and concatenate 5 copies ffmpeg -f lavfi -i smptebars=duration=2.08:size=1920x1080:rate=24 \ -codec dnxhd -pix_fmt yuv422p -b:v 115M smptebars_dnx_1000.mov cat < concat_1000.txt file smptebars_dnx_1000.mov file smptebars_dnx_1000.mov file smptebars_dnx_1000.mov file smptebars_dnx_1000.mov file smptebars_dnx_1000.mov EOF ffmpeg -f concat -i concat_1000.txt -c copy smpte_concat_1000.mov ffprobe smpte_concat_1000.mov The output of ffprobe will show a frame rate of 23.99 due to the effect of the sample durations in the stts entries. With the new option of -mov_timescale set to 600: ffmpeg -f lavfi -i smptebars=duration=2.08:size=1920x1080:rate=24 \ -codec dnxhd -pix_fmt yuv422p -b:v 115M -mov_timescale 600 smptebars_dnx_600.mov cat < concat_600.txt file smptebars_dnx_600.mov file smptebars_dnx_600.mov file smptebars_dnx_600.mov file smptebars_dnx_600.mov file smptebars_dnx_600.mov EOF ffmpeg -f concat -i concat_600.txt -c copy -mov_timescale 600 smpte_concat_600.mov ffprobe smpte_concat_600.mov The durations all line up, the stts table is smaller and no rounding issues occur. Kevin Wheatley (2): avformat/movenc: Add command line option to set base mov file timescale avformat/movenc: Use base container timescale, instead of hard coded default libavformat/movenc.c | 23 --- libavformat/movenc.h | 1 + 2 files changed, 13 insertions(+), 11 deletions(-) -- 1.8.5.6 0002-avformat-movenc-Use-base-container-timescale-instead.patch Description: Binary data 0001-avformat-movenc-Add-command-line-option-to-set-base-.patch Description: Binary data ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] colorspace: Rename jedec-p22 to ebu3213
On Fri, Aug 9, 2019 at 12:40 PM Hendrik Leppkes wrote: > The enum and our values are aligned to the H.273 / ISO/IEC 23001-8 > standards, which documents this entry to correspond to the primaries > in use by vf_colorspace That makes sense, although I'm now interested to find out where those numbers came from as they didn't come out of the EBU document, guess I'll have to ask next time I bump into somebody from the ITU or EBU, the EBU numbers do come with allowable tolerances, but the blue co-ordinate appears to fall outside of that. Thanks Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] colorspace: Rename jedec-p22 to ebu3213
Is there a change to include the EBU primaries? https://tech.ebu.ch/docs/tech/tech3213.pdf White 0.313 0.329 Red 0.64 0.33 Green 0.29 0.60 Blue 0.15 0.06 as the ones currently called AVCOL_PRI_JEDEC_P22 are not those ones at least in vf_colorspace.c [AVCOL_PRI_JEDEC_P22] = { WP_D65, { 0.630, 0.340, 0.295, 0.605, 0.155, 0.077 } }, Kevin On Fri, Aug 9, 2019 at 1:48 AM James Almer wrote: > > On 8/8/2019 9:01 PM, rzu...@tebako.net wrote: > > From: Raphaël Zumer > > > > Internally, this adds an EBU3213 alias to JEDEC_P22, > > and changes the name string to match ITU-T H.273. > > --- > > libavcodec/options_table.h | 2 +- > > libavfilter/vf_colorspace.c | 2 +- > > libavfilter/vf_setparams.c | 2 +- > > libavfilter/vf_zscale.c | 2 +- > > libavutil/pixdesc.c | 2 +- > > libavutil/pixfmt.h | 1 + > > 6 files changed, 6 insertions(+), 5 deletions(-) > > This should be split into three patches. The first one adding the enum > value to pixfmt.h, changing the string in pixdesc.c, bumping libavutil > minor version, and adding an entry to doc/APIChanges. The subject should > be something like "avutil/pixfmt: Add EBU Tech. 3213-E AVColorPrimaries > value" plus the explanation that it's an alias for Jedec P22 (Or that > one becoming an alias for the new one) after it. > Then another patch changing the libavfilter option values and bumping > libavfilter micro version, and another doing the same with libavcodec. > > > > > diff --git a/libavcodec/options_table.h b/libavcodec/options_table.h > > index 4a266eca16..9d82188171 100644 > > --- a/libavcodec/options_table.h > > +++ b/libavcodec/options_table.h > > @@ -365,7 +365,7 @@ static const AVOption avcodec_options[] = { > > {"smpte428_1", "SMPTE 428-1",0, AV_OPT_TYPE_CONST, {.i64 = > > AVCOL_PRI_SMPTE428 }, INT_MIN, INT_MAX, V|E|D, "color_primaries_type"}, > > {"smpte431","SMPTE 431-2",0, AV_OPT_TYPE_CONST, {.i64 = > > AVCOL_PRI_SMPTE431 }, INT_MIN, INT_MAX, V|E|D, "color_primaries_type"}, > > {"smpte432","SMPTE 422-1",0, AV_OPT_TYPE_CONST, {.i64 = > > AVCOL_PRI_SMPTE432 }, INT_MIN, INT_MAX, V|E|D, "color_primaries_type"}, > > -{"jedec-p22", "JEDEC P22", 0, AV_OPT_TYPE_CONST, {.i64 = > > AVCOL_PRI_JEDEC_P22 },INT_MIN, INT_MAX, V|E|D, "color_primaries_type"}, > > +{"ebu3213", "EBU 3213-E", 0, AV_OPT_TYPE_CONST, {.i64 = > > AVCOL_PRI_JEDEC_P22 },INT_MIN, INT_MAX, V|E|D, "color_primaries_type"}, > > {"unspecified", "Unspecified",0, AV_OPT_TYPE_CONST, {.i64 = > > AVCOL_PRI_UNSPECIFIED }, INT_MIN, INT_MAX, V|E|D, "color_primaries_type"}, > > {"color_trc", "color transfer characteristics", OFFSET(color_trc), > > AV_OPT_TYPE_INT, {.i64 = AVCOL_TRC_UNSPECIFIED }, 1, INT_MAX, V|E|D, > > "color_trc_type"}, > > {"bt709","BT.709", 0, AV_OPT_TYPE_CONST, {.i64 = > > AVCOL_TRC_BT709 },INT_MIN, INT_MAX, V|E|D, "color_trc_type"}, > > diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c > > index df6efffb3d..5f22f92507 100644 > > --- a/libavfilter/vf_colorspace.c > > +++ b/libavfilter/vf_colorspace.c > > @@ -968,7 +968,7 @@ static const AVOption colorspace_options[] = { > > ENUM("smpte431", AVCOL_PRI_SMPTE431, "prm"), > > ENUM("smpte432", AVCOL_PRI_SMPTE432, "prm"), > > ENUM("bt2020", AVCOL_PRI_BT2020, "prm"), > > -ENUM("jedec-p22",AVCOL_PRI_JEDEC_P22, "prm"), > > +ENUM("ebu3213", AVCOL_PRI_JEDEC_P22, "prm"), > > > > { "trc","Output transfer characteristics", > >OFFSET(user_trc), AV_OPT_TYPE_INT, { .i64 = AVCOL_TRC_UNSPECIFIED > > }, > > diff --git a/libavfilter/vf_setparams.c b/libavfilter/vf_setparams.c > > index fe298e5a06..80e61f851e 100644 > > --- a/libavfilter/vf_setparams.c > > +++ b/libavfilter/vf_setparams.c > > @@ -74,7 +74,7 @@ static const AVOption setparams_options[] = { > > {"smpte428",NULL, 0, AV_OPT_TYPE_CONST, > > {.i64=AVCOL_PRI_SMPTE428}, INT_MIN, INT_MAX, FLAGS, "color_primaries"}, > > {"smpte431",NULL, 0, AV_OPT_TYPE_CONST, > > {.i64=AVCOL_PRI_SMPTE431}, INT_MIN, INT_MAX, FLAGS, "color_primaries"}, > > {"smpte432",NULL, 0, AV_OPT_TYPE_CONST, > > {.i64=AVCOL_PRI_SMPTE432}, INT_MIN, INT_MAX, FLAGS, "color_primaries"}, > > -{"jedec-p22", NULL, 0, AV_OPT_TYPE_CONST, > > {.i64=AVCOL_PRI_JEDEC_P22},INT_MIN, INT_MAX, FLAGS, "color_primaries"}, > > +{"ebu3213", NULL, 0, AV_OPT_TYPE_CONST, > > {.i64=AVCOL_PRI_JEDEC_P22},INT_MIN, INT_MAX, FLAGS, "color_primaries"}, > > > > {"color_trc", "select color transfer", OFFSET(color_trc), > > AV_OPT_TYPE_INT, {.i64=-1}, -1, AVCOL_TRC_NB-1, FLAGS, "color_trc"}, > > {"auto", "keep the same color transfer", 0, AV_OPT_TYPE_CONST, > > {.i64=-1}, INT_MIN, INT_MAX, FLAGS, "color_trc"}, > > diff --git a/libavfilter/
Re: [FFmpeg-devel] avfilter/vf_ciescope: add DCI-P3
Just a query, but I note that the gamma says 'GAMMA_REC709', this is not the normal gamma I'd expect for a DCI P3 image, it would typically be 2.6, so for correct translation into chromaticities, you would need to use that to convert to linear RGB. It is also not the only option for encoding the output image, although it appears it is the only option listed in the structure. What is that gamma parameter in the colour space structure supposed to represent? Thanks Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [RFC]lavc/mjpegenc_common: Fix aspect ratio
I guess it depends if you think that it is better to align with defacto behaviour (and maybe clarify/correct the specifications) or follow the specs and have users grumble about not matching the behaviour of 'applications X, Y and Z', I'm pretty certain Photoshop won't easily change its behaviour. The other messy option would be to have a compatibility mode flag. I don't think any of this is perfect! Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC]lavc/mjpegenc_common: Fix aspect ratio
I came across a similar discussion here: https://github.com/OpenImageIO/oiio/pull/1412 Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] web/index: add news entry for release 4.1
OK, that makes sense I had wondered if it was a typo with d and x being close to each other on the keyboard. and might be better written as one line. Obviously not! Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] web/index: add news entry for release 4.1
On Wed, Nov 14, 2018 at 4:40 PM James Almer wrote: > +colorconstancy filter > +AVS2 video decoder via libdavs2 [snip] > +AVS2 video encoder via libxavs2 This line is in twice. Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] libavfilter/vf_libvmaf.c The libvmaf filter tried to join on an invalid thread id
On Thu, May 3, 2018 at 12:38 PM, Ronald S. Bultje wrote: > Why? > > Your patch fixes a bug, I don't think we test bugs in fate, just features. I am perhaps making an incorrect assumption that the code has a bug because it is never tested and that by adding a test that simply tries to use it, the vmaf code is more likely to work. I'm happy to not write a test if that is preferred! Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] libavfilter/vf_libvmaf.c The libvmaf filter tried to join on an invalid thread id
Following up my own email with another question or so: Could somebody point me at a suitable method of testing this within the Fate framework? I've started to try unpick how the fate tests are run, etc What I need is a prototypical example which presumably would need to factor in (what I figured out so far): libvmaf is an optional component so the test should be too. The test should probably live in tests/fate/filter-video.mak The function (CMD) called should live in tests/fate-run.sh? For the actual test I need two inputs an original and an encoded frame (sequence is ok too), that can then feed into the vmaf invocation of ffmpeg. Is there a set of inputs I can just reuse from what fate would normally run so that the test need only run ffmpeg to compute the output of vmaf, (or is the preference for every test to generate its own input?) Thanks Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] libavfilter/vf_libvmaf.c The libvmaf filter tried to join on an invalid thread id
Following on from my report in the user list here is a better than a quick hack suggestion to avoid trying to join on an invalid thread id. This may not be the best solution, but it does avoid the SEGV on linux when calling pthread_join() Kevin From a94d0cb7ff4202487ea980a029fa613c58d6d7c3 Mon Sep 17 00:00:00 2001 From: Kevin Wheatley Date: Mon, 30 Apr 2018 16:33:51 +0100 Subject: [PATCH] The libvmaf filter tried to join on an invalid thread id The thread id was invalid because it was not initialised during the calls to init_complex_filtergraph. This adds a flag to check for initialisation before trying to peform the join. Signed-off-by: Kevin Wheatley --- libavfilter/vf_libvmaf.c |9 - 1 files changed, 8 insertions(+), 1 deletions(-) diff --git a/libavfilter/vf_libvmaf.c b/libavfilter/vf_libvmaf.c index 42c6b66..5d47a74 100644 --- a/libavfilter/vf_libvmaf.c +++ b/libavfilter/vf_libvmaf.c @@ -43,6 +43,7 @@ typedef struct LIBVMAFContext { int width; int height; double vmaf_score; +int vmaf_thread_created; pthread_t vmaf_thread; pthread_mutex_t lock; pthread_cond_t cond; @@ -228,6 +229,7 @@ static av_cold int init(AVFilterContext *ctx) s->gmain = av_frame_alloc(); s->error = 0; +s->vmaf_thread_created = 0; pthread_mutex_init(&s->lock, NULL); pthread_cond_init (&s->cond, NULL); @@ -275,6 +277,7 @@ static int config_input_ref(AVFilterLink *inlink) av_log(ctx, AV_LOG_ERROR, "Thread creation failed.\n"); return AVERROR(EINVAL); } +s->vmaf_thread_created = 1; return 0; } @@ -317,7 +320,11 @@ static av_cold void uninit(AVFilterContext *ctx) pthread_cond_signal(&s->cond); pthread_mutex_unlock(&s->lock); -pthread_join(s->vmaf_thread, NULL); +if (s->vmaf_thread_created) +{ +pthread_join(s->vmaf_thread, NULL); +s->vmaf_thread_created = 0; +} av_frame_free(&s->gref); av_frame_free(&s->gmain); -- 1.7.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/4] avformat/movenc: force colr atom for uncompressed yuv in mov
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 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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] libavcodec/exr: Fix blank output when data window != display window
No, just this kind. Behind the images when data = display window, the most common case OpenEXR file we have, is with a reduced data window that resides completely inside the display window, though this particular bug would impact any files where the value of ymax+1 is not equal to the height. Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] libavcodec/exr: Fix blank output when data window != display window
The following is a sample EXR that needs the patch to correctly process in FFmpeg. Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] libavcodec/exr: Fix blank output when data window != display window
looks like there is a bug in commit 1a08758e7c4e14a9ea8d2fef6c33ad411b2d3c40 relating to the handling of ptr in decode_frame after decode_block is called, before this commit ptr would have been incremented for each line in the data window, now after the commit it is left at the start of the first included line rather than the line after the data window then the code sets the remaining lines to 0 and thus the whole image is over written. Fix by adjusting ptr to the correct line after decode_block returns Signed-off-by: Kevin Wheatley 0001-libavcodec-exr-Fix-blank-output-when-data-window-dis.patch Description: Binary data ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] vf_colorspace: Add support for smpte 431/432 (dci/display p3) primaries
I would really strongly suggest including DCI in the name at least - though nobody else would choose to use it for anything other than the reference calibration - most titles use a creative white different to that of the encoding reference (one that is less green). Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] vf_colorspace: Add support for smpte 431/432 (dci/display p3) primaries
On Sun, Oct 30, 2016 at 1:18 PM, Ronald S. Bultje wrote: > Hmm... So, the wikipedia page https://en.wikipedia.org/wiki/DCI-P3 refers > to the two whitepoints here as DCI-P3 D65 and DCI-P3 Theater. Calling one > D65 and the other DCI seems confusing in that light (assuming the wikipedia > page is correct). I'd call it THEATER or DCI_P3_THEATER, to distinguish it > from DCI-P3 D65. Is that OK? In the industry people just call it the DCI P3 white point (or 'Urgh') it is not limited to theater usage, you might consider it the calibration white point for the reference projector, so WP_DCI_P3_REFERENCE might be better, but that is a little long. I'd go for something like WP_DCI_P3 it is not really ambiguous. Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] avformat/mov: improve qt metadata reading and writing
On Thu, Jun 23, 2016 at 10:44 AM, Michael Niedermayer wrote: > that was maybe forgotten due to the rfc in the subject I never pushed for it as our internal requirement for the feature went away and I only try to push things we actually found useful :-) the patch probably won't merge cleanly now anyway. The other part of the problem is ideally I'd like to output the metadata correctly, but to do that ffmpeg would need to understand multiple data types better (or at least when I looked at it last this was the case). I also remember there maybe similar comments about libav needing more robust metadata support too. Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] Avid DNxHR / UK DPP
On Tue, Jan 26, 2016 at 5:30 PM, John Warburton wrote: > I'm at DPP conference, in the room. Start of UHD delivery standards for UK > just announced. Now on website, we are told. https://www.digitalproductionpartnership.co.uk/what-we-do/technical-standards/ http://dpp-assets.s3.amazonaws.com/wp-content/uploads/2016/01/TechnicalDeliverySupplementUHDDPP.pdf is the template for broadcasters to fill in the blanks. Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avfilter: add zscale filter
On Wed, Sep 30, 2015 at 9:49 AM, Paul B Mahol wrote: > +{ "range", "set color range", OFFSET(range), AV_OPT_TYPE_INT, > {.i64 = -1}, -1, ZIMG_RANGE_FULL, FLAGS, "range" }, > +{ "r", "set color range", OFFSET(range), AV_OPT_TYPE_INT, > {.i64 = -1}, -1, ZIMG_RANGE_FULL, FLAGS, "range" }, > +{ "input",0, 0, AV_OPT_TYPE_CONST, > {.i64 = -1}, 0, 0, FLAGS, "range" }, > +{ "limited", 0, 0, AV_OPT_TYPE_CONST, > {.i64 = ZIMG_RANGE_LIMITED}, 0, 0, FLAGS, "range" }, > +{ "full", 0, 0, AV_OPT_TYPE_CONST, > {.i64 = ZIMG_RANGE_FULL},0, 0, FLAGS, "range" }, > +{ "primaries", "set color primaries", OFFSET(primaries), > AV_OPT_TYPE_INT, {.i64 = -1}, -1, ZIMG_PRIMARIES_2020, FLAGS, "primaries" }, > +{ "p", "set color primaries", OFFSET(primaries), > AV_OPT_TYPE_INT, {.i64 = -1}, -1, ZIMG_PRIMARIES_2020, FLAGS, "primaries" }, > +{ "input",0, 0, AV_OPT_TYPE_CONST, > {.i64 = -1}, 0, 0, FLAGS, "primaries" }, > +{ "709", 0, 0, AV_OPT_TYPE_CONST, > {.i64 = ZIMG_PRIMARIES_709}, 0, 0, FLAGS, "primaries" }, > +{ "unspecified", 0, 0, AV_OPT_TYPE_CONST, > {.i64 = ZIMG_PRIMARIES_UNSPECIFIED}, 0, 0, FLAGS, "primaries" }, > +{ "170m", 0, 0, AV_OPT_TYPE_CONST, > {.i64 = ZIMG_PRIMARIES_170M},0, 0, FLAGS, "primaries" }, > +{ "240m", 0, 0, AV_OPT_TYPE_CONST, > {.i64 = ZIMG_PRIMARIES_240M},0, 0, FLAGS, "primaries" }, > +{ "2020", 0, 0, AV_OPT_TYPE_CONST, > {.i64 = ZIMG_PRIMARIES_2020},0, 0, FLAGS, "primaries" }, > +{ "transfer", "set transfer characteristic", OFFSET(trc), > AV_OPT_TYPE_INT, {.i64 = -1}, -1, ZIMG_TRANSFER_2020_12, FLAGS, "transfer" }, > +{ "t","set transfer characteristic", OFFSET(trc), > AV_OPT_TYPE_INT, {.i64 = -1}, -1, ZIMG_TRANSFER_2020_12, FLAGS, "transfer" }, > +{ "input",0, 0, AV_OPT_TYPE_CONST, > {.i64 = -1}, 0, 0, FLAGS, "transfer" }, > +{ "709", 0, 0, AV_OPT_TYPE_CONST, > {.i64 = ZIMG_TRANSFER_709}, 0, 0, FLAGS, "transfer" }, > +{ "unspecified", 0, 0, AV_OPT_TYPE_CONST, > {.i64 = ZIMG_TRANSFER_UNSPECIFIED}, 0, 0, FLAGS, "transfer" }, > +{ "601", 0, 0, AV_OPT_TYPE_CONST, > {.i64 = ZIMG_TRANSFER_601}, 0, 0, FLAGS, "transfer" }, > +{ "linear", 0, 0, AV_OPT_TYPE_CONST, > {.i64 = ZIMG_TRANSFER_LINEAR}, 0, 0, FLAGS, "transfer" }, > +{ "2020_10", 0, 0, AV_OPT_TYPE_CONST, > {.i64 = ZIMG_TRANSFER_2020_10}, 0, 0, FLAGS, "transfer" }, > +{ "2020_12", 0, 0, AV_OPT_TYPE_CONST, > {.i64 = ZIMG_TRANSFER_2020_12}, 0, 0, FLAGS, "transfer" }, > +{ "matrix", "set colorspace matrix", OFFSET(colorspace), > AV_OPT_TYPE_INT, {.i64 = -1}, -1, ZIMG_MATRIX_2020_CL, FLAGS, "matrix" }, > +{ "m", "set colorspace matrix", OFFSET(colorspace), > AV_OPT_TYPE_INT, {.i64 = -1}, -1, ZIMG_MATRIX_2020_CL, FLAGS, "matrix" }, > +{ "input",0, 0, AV_OPT_TYPE_CONST, > {.i64 = -1}, 0, 0, FLAGS, "matrix" }, > +{ "709", 0, 0, AV_OPT_TYPE_CONST, > {.i64 = ZIMG_MATRIX_709},0, 0, FLAGS, "matrix" }, > +{ "unspecified", 0, 0, AV_OPT_TYPE_CONST, > {.i64 = ZIMG_MATRIX_UNSPECIFIED},0, 0, FLAGS, "matrix" }, > +{ "470bg",0, 0, AV_OPT_TYPE_CONST, > {.i64 = ZIMG_MATRIX_470BG}, 0, 0, FLAGS, "matrix" }, > +{ "170m", 0, 0, AV_OPT_TYPE_CONST, > {.i64 = ZIMG_MATRIX_170M}, 0, 0, FLAGS, "matrix" }, > +{ "2020_ncl", 0, 0, AV_OPT_TYPE_CONST, > {.i64 = ZIMG_MATRIX_2020_NCL}, 0, 0, FLAGS, "matrix" }, > +{ "2020_cl", 0, 0, AV_OPT_TYPE_CONST, > {.i64 = ZIMG_MATRIX_2020_CL},0, 0, FLAGS, "matrix" }, As a casual developer/observer, I wonder about the consistency of the options here with other parts of ffmpeg, in the general case it uses: -color_primaries bt709 unspecified bt470m bt470bg smpte170m smpte240m film bt2020 -color_trc bt709 unspecified gamma22 gamma28 smpte170m smpte240m linear log log_sqrt iec61966_2_4 bt1361 iec61966_2_1 bt2020_10bit bt2020_12bit -colorspace rgb
Re: [FFmpeg-devel] [PATCH][RFC] libavformat/mov Extend metadata handling to read in the keys from the 'keys' atom
I've updated and extended the metadata reading to include some of the numeric types, based on the feedback from the comments about adding to the utility functions of AVDictionary, I've directly formatted the values as strings in the .mov format code seams less risky. Kevin From 91515aec2e964e0fb499378e31cd7782bf93482a Mon Sep 17 00:00:00 2001 From: Kevin Wheatley Date: Thu, 3 Sep 2015 15:58:49 +0100 Subject: [PATCH] avformat/mov: Extend metadata handling to read in the keys from the 'keys' atom Signed-off-by: Kevin Wheatley --- libavformat/isom.h |1 + libavformat/mov.c | 63 2 files changed, 64 insertions(+), 0 deletions(-) diff --git a/libavformat/isom.h b/libavformat/isom.h index aee9d6e..8c20ec9 100644 --- a/libavformat/isom.h +++ b/libavformat/isom.h @@ -206,6 +206,7 @@ typedef struct MOVContext { void *audible_fixed_key; int audible_fixed_key_size; struct AVAES *aes_decrypt; +AVDictionary *keys_d; // Use a dictionary as an array a little bit ugly } MOVContext; int ff_mp4_read_descr_len(AVIOContext *pb); diff --git a/libavformat/mov.c b/libavformat/mov.c index 92d90db..46c4b17 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -265,6 +265,7 @@ static int mov_read_udta_string(MOVContext *c, AVIOContext *pb, MOVAtom atom) uint32_t data_type = 0, str_size, str_size_alloc; int (*parse)(MOVContext*, AVIOContext*, unsigned, const char*) = NULL; int raw = 0; +AVDictionaryEntry *entry = NULL; switch (atom.type) { case MKTAG( '@','P','R','M'): key = "premiere_version"; raw = 1; break; @@ -351,6 +352,13 @@ static int mov_read_udta_string(MOVContext *c, AVIOContext *pb, MOVAtom atom) case MKTAG(0xa9,'w','r','n'): key = "warning"; break; case MKTAG(0xa9,'w','r','t'): key = "composer"; break; case MKTAG(0xa9,'x','y','z'): key = "location"; break; +default: +snprintf(key2, sizeof(key2), "%08x", atom.type); +entry = av_dict_get(c->keys_d, key2, entry, 0); +if (entry) +key = entry->value; +av_log(c->fc, AV_LOG_TRACE, "Attempting to match unknown atom type %08x %s in keys => '%s'\n", atom.type, key2, key); +break; } retry: if (c->itunes_metadata && atom.size > 8) { @@ -3184,6 +3192,58 @@ static int mov_read_meta(MOVContext *c, AVIOContext *pb, MOVAtom atom) return 0; } +static int mov_read_keys(MOVContext *c, AVIOContext *pb, MOVAtom atom) +{ +AVDictionaryEntry *entry = NULL; +uint32_t key_count, key_size, key_namespace, key_num; +char *key_str; +char key_num_str[8+1] = { 0 }; // Use hexadecimal string of a 32 bit integer + terminator +int ret; + +avio_r8(pb); /* version */ +avio_rb24(pb); /* flags */ +key_count = avio_rb32(pb); + +av_log(c->fc, AV_LOG_VERBOSE, + "Reading keys sz: %"PRId64" %u\n", atom.size, key_count); + +key_num = 1; // Keys are numbered from 1 and refered to in the ilst following. +while (key_count--) { +key_size = avio_rb32(pb) - 8; +key_namespace = avio_rl32(pb); +if (key_namespace == MKTAG('m','d','t','a')) { +key_str = av_malloc(key_size + 1); /* Add null terminator */ +if (!key_str) { +return AVERROR(ENOMEM); +} + +ret = ffio_read_size(pb, key_str, key_size); +if (ret < 0) { +av_freep(&key_str); +return ret; +} + +key_str[key_size] = 0; +if (key_str[0]) { +snprintf(key_num_str, sizeof(key_num_str), "%08x", av_be2ne32(key_num)); +av_dict_set(&(c->keys_d), key_num_str, key_str, 0); +} +av_freep(&key_str); +} else { +// Don't know what to do with other namespaces... ignore +avio_seek(pb, key_size, SEEK_CUR); +} +++key_num; +} + +while (entry = av_dict_get(c->keys_d, "", entry, AV_DICT_IGNORE_SUFFIX)) { +av_log(c->fc, AV_LOG_TRACE, + "Read metadata key %s: '%s'\n", entry->key, entry->value); +} + +return 0; +} + static int mov_read_tkhd(MOVContext *c, AVIOContext *pb, MOVAtom atom) { int i; @@ -3786,6 +3846,7 @@ static const MOVParseTableEntry mov_default_parse_table[] = { { MKTAG('h','d','l','r'), mov_read_hdlr }, { MKTAG('i','l','s','t'), mov_read_ilst }, { MKTAG('j','p','2','h'), mov_read_j
Re: [FFmpeg-devel] [PATCH] libavutil/dict: extend the list of convienience functions for storing different data types
On Fri, Sep 4, 2015 at 4:18 PM, wm4 wrote: > On Fri, 4 Sep 2015 14:38:54 +0100 > Kevin Wheatley wrote: > >> Hi, >> >> as part of adding support for non-string data types to .mov metadata, >> I wondered about adding the following helper functions for storing >> numeric types into an AVDictionary. >> >> upfront I'll say I'm not 100% happy with the float32 and float64 named >> variants (vs float and double) as there is no clear preference in >> other areas of the code that I could see. > > I don't understand it either. Why not just use float and double, > instead of obfuscating the names further? (It'd be a difference if this > function e.g. serialized the floats to binary - but it literally just > passes them as-is to libc. Having the C data type in the argument seems > advantageous. > > Also, what's the use in having a float function at all? > > I'd call av_dict_set_uint av_dict_set_uint64. I'll explain my POV... Essentially these helpers do the same as the existing av_dict_set_int() does for int64_t so I've essentially just copied that and substituted the required conversions from to char array. The floating point is needed to correctly decode those formats. Using the bit length in the name of the floating point variation of the functions is because they will only correctly produce the textual representation sufficient to transform back into the same binary representation based on the bit lengths of the floating point representation (they assume IEEE single and double precision in the lengths of the formatting options and thus buffer size). Mentally I was in the middle of the Apple QuickTime metadata representations where they also use the floatXX terminology - not a great excuse I know :-) Re Nicholas' comments on the flags &= ~AV_DICT_DONT_STRDUP_VAL; I was simply replicating the existing form of the function av_dict_set_int(). As to portability in the tests - yes I agree - happy for somebody to point me towards a good solution for that. My alternative to these would have been to embed the formatting in the libavformat/mov.c code which obviously would have less impact. Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] libavutil/dict: extend the list of convienience functions for storing different data types
Hi, as part of adding support for non-string data types to .mov metadata, I wondered about adding the following helper functions for storing numeric types into an AVDictionary. upfront I'll say I'm not 100% happy with the float32 and float64 named variants (vs float and double) as there is no clear preference in other areas of the code that I could see. I'm also conscious that to provide for rewriting the .mov metadata types, I'll need something different to store the actual types and that might mean these functions would end up not being used at all. Kevin From 34fedb67d58402b519a7c91bff7623469802c4c4 Mon Sep 17 00:00:00 2001 From: Kevin Wheatley Date: Fri, 4 Sep 2015 14:26:49 +0100 Subject: [PATCH] libavutil/dict: extend the list of convienience functions for storing different data types Signed-off-by: Kevin Wheatley --- libavutil/dict.c| 81 +- libavutil/dict.h| 24 +++ tests/ref/fate/dict | 17 +++ 3 files changed, 120 insertions(+), 2 deletions(-) diff --git a/libavutil/dict.c b/libavutil/dict.c index 6ff1af5..4eaaf83 100644 --- a/libavutil/dict.c +++ b/libavutil/dict.c @@ -149,6 +149,33 @@ int av_dict_set_int(AVDictionary **pm, const char *key, int64_t value, return av_dict_set(pm, key, valuestr, flags); } +int av_dict_set_uint(AVDictionary **pm, const char *key, uint64_t value, +int flags) +{ +char valuestr[22]; +snprintf(valuestr, sizeof(valuestr), "%"PRIu64, value); +flags &= ~AV_DICT_DONT_STRDUP_VAL; +return av_dict_set(pm, key, valuestr, flags); +} + +int av_dict_set_float32(AVDictionary **pm, const char *key, float value, +int flags) +{ +char valuestr[17]; // sign + 1 + point + 8 + e + sign + 3 + term +snprintf(valuestr, sizeof(valuestr), "%.9g", value); +flags &= ~AV_DICT_DONT_STRDUP_VAL; +return av_dict_set(pm, key, valuestr, flags); +} + +int av_dict_set_float64(AVDictionary **pm, const char *key, double value, +int flags) +{ +char valuestr[26]; // sign + 1 + point + 16 + e + sign + 4 + term +snprintf(valuestr, sizeof(valuestr), "%.17g", value); +flags &= ~AV_DICT_DONT_STRDUP_VAL; +return av_dict_set(pm, key, valuestr, flags); +} + static int parse_key_value_pair(AVDictionary **pm, const char **buf, const char *key_val_sep, const char *pairs_sep, int flags) @@ -276,9 +303,13 @@ static void test_separators(const AVDictionary *m, const char pair, const char v int main(void) { AVDictionary *dict = NULL; -AVDictionaryEntry *e; +AVDictionaryEntry *e, *e2; char *buffer = NULL; - +float f32 = 0.0f; +double f64 = 0.0f; +union { uint32_t i; float f; } intfloat; +union { uint64_t ui; int64_t i; } un_signed_int; + printf("Testing av_dict_get_string() and av_dict_parse_string()\n"); av_dict_get_string(dict, &buffer, '=', ','); printf("%s\n", buffer); @@ -356,6 +387,52 @@ int main(void) printf("%s\n", e->value); av_dict_free(&dict); +printf("\nTesting av_dict_set_int()\n"); +un_signed_int.ui = 0U; +av_dict_set_int(&dict, "0", un_signed_int.i, 0); +un_signed_int.ui = 0x8000UL; +av_dict_set_int(&dict, "-max", un_signed_int.i, 0); +un_signed_int.ui = 0x7fffUL; +av_dict_set_int(&dict, "max", un_signed_int.i, 0); +e = NULL; +while ((e = av_dict_get(dict, "", e, AV_DICT_IGNORE_SUFFIX))) +printf("%s %s\n", e->key, e->value); +av_dict_free(&dict); + +printf("\nTesting av_dict_set_uint()\n"); +av_dict_set_uint(&dict, "0", 0, 0); +av_dict_set_uint(&dict, "max", 0xUL, 0); +e = NULL; +while ((e = av_dict_get(dict, "", e, AV_DICT_IGNORE_SUFFIX))) +printf("%s %s\n", e->key, e->value); +av_dict_free(&dict); + +printf("\nTesting av_dict_set_float*()\n"); +// float and double representation of a given number should give the same value +f32 = -1.20786635e-05f; +f64 = -1.20786635e-05; +av_dict_set_float32(&dict, "f32", f32, 0); +e = av_dict_get(dict, "f32", NULL, 0); +printf("%.30g => %s\n", f32, e->value); +av_dict_set_float64(&dict, "f64", f64, 0); +e2 = av_dict_get(dict, "f64", NULL, 0); +printf("%.30g => %s\n", f64, e2->value); +printf("%s == %s %s\n", e->value, e2->value, strcmp(e->value, e2->value) == 0 ? "OK" : "BAD"); + +intfloat.i = 0xa647609; +av_dict_set_float32(&dict, "0xa647609", intfloat.f, 0); +e =
Re: [FFmpeg-devel] [PATCH][RFC] libavformat/mov Extend metadata handling to read in the keys from the 'keys' atom
On Thu, Sep 3, 2015 at 7:49 PM, Michael Niedermayer wrote: > missing checks for interger overflows of the addition and subtraction > > also the subject says "RFC", is there a reason not to push this to > git master once it otherwise looks good ? it is incomplete, basically I was fishing for a general OK to (ab)use the dictionary in the MOVContext as a means to pass on the list of keys, having done that, I've moved on to extend the data types understood to include other data types (The sample ARRI clips need signed and unsigned integers). I have this working but the code is in need of a refactoring to say the least. There does not appear to be a way to indicate the 'type' of data pushed into the dictionary so that if I wanted to serialise it back out I could get the same format, is this something people have considered? I could use a type dictionary to store the original type of the keys along side the standard metatdata dictionary, but how that would feed into the metatdata mux/demuxing and the command line interface I don't know. Thanks Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH][RFC] libavformat/mov Extend metadata handling to read in the keys from the 'keys' atom
Looking for feedback on this patch, in particular the use of the 'keys' dictionary within the MOVContext structure. With the patch FFmpeg can now 'find' the metadata in ARRI's sample footage see here: http://www.arri.com/camera/alexa/learn/alexa_sample_footage/ Thanks Kevin From 57feef09b636d8d80d3afe7dca20175ddb51 Mon Sep 17 00:00:00 2001 From: Kevin Wheatley Date: Thu, 3 Sep 2015 15:58:49 +0100 Subject: [PATCH] RFC: Extend metadata handling to read in the keys from the 'keys' atom --- libavformat/isom.h |1 + libavformat/mov.c | 63 2 files changed, 64 insertions(+), 0 deletions(-) diff --git a/libavformat/isom.h b/libavformat/isom.h index aee9d6e..8c20ec9 100644 --- a/libavformat/isom.h +++ b/libavformat/isom.h @@ -206,6 +206,7 @@ typedef struct MOVContext { void *audible_fixed_key; int audible_fixed_key_size; struct AVAES *aes_decrypt; +AVDictionary *keys_d; // Use a dictionary as an array a little bit ugly } MOVContext; int ff_mp4_read_descr_len(AVIOContext *pb); diff --git a/libavformat/mov.c b/libavformat/mov.c index 45367d3..1cf50bb 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -265,6 +265,7 @@ static int mov_read_udta_string(MOVContext *c, AVIOContext *pb, MOVAtom atom) uint32_t data_type = 0, str_size, str_size_alloc; int (*parse)(MOVContext*, AVIOContext*, unsigned, const char*) = NULL; int raw = 0; +AVDictionaryEntry *entry = NULL; switch (atom.type) { case MKTAG( '@','P','R','M'): key = "premiere_version"; raw = 1; break; @@ -351,6 +352,13 @@ static int mov_read_udta_string(MOVContext *c, AVIOContext *pb, MOVAtom atom) case MKTAG(0xa9,'w','r','n'): key = "warning"; break; case MKTAG(0xa9,'w','r','t'): key = "composer"; break; case MKTAG(0xa9,'x','y','z'): key = "location"; break; +default: +snprintf(key2, sizeof(key2), "%08x", atom.type); +entry = av_dict_get(c->keys_d, key2, entry, 0); +if (entry) +key = entry->value; +av_log(c->fc, AV_LOG_TRACE, "Attempting to match unknown atom type %08x %s in keys => '%s'\n", atom.type, key2, key); +break; } retry: if (c->itunes_metadata && atom.size > 8) { @@ -3184,6 +3192,58 @@ static int mov_read_meta(MOVContext *c, AVIOContext *pb, MOVAtom atom) return 0; } +static int mov_read_keys(MOVContext *c, AVIOContext *pb, MOVAtom atom) +{ +AVDictionaryEntry *entry = NULL; +uint32_t key_count, key_size, key_namespace, key_num; +char *key_str; +char key_num_str[8+1] = { 0 }; // Use hexadecimal string of a 32 bit integer + terminator +int ret; + +avio_r8(pb); /* version */ +avio_rb24(pb); /* flags */ +key_count = avio_rb32(pb); + +av_log(c->fc, AV_LOG_VERBOSE, + "Reading keys sz: %"PRId64" %u\n", atom.size, key_count); + +key_num = 1; // Keys are numbered from 1 and refered to in the ilst following. +while (key_count--) { +key_size = avio_rb32(pb) - 8; +key_namespace = avio_rl32(pb); +if (key_namespace == MKTAG('m','d','t','a')) { +key_str = av_malloc(key_size + 1); /* Add null terminator */ +if (!key_str) { +return AVERROR(ENOMEM); +} + +ret = ffio_read_size(pb, key_str, key_size); +if (ret < 0) { +av_freep(&key_str); +return ret; +} + +key_str[key_size] = 0; +if (key_str[0]) { +snprintf(key_num_str, sizeof(key_num_str), "%08x", av_bswap32(key_num)); +av_dict_set(&(c->keys_d), key_num_str, key_str, 0); +} +av_freep(&key_str); +} else { +// Don't know what to do with other namespaces... ignore +avio_seek(pb, key_size, SEEK_CUR); +} +++key_num; +} + +while (entry = av_dict_get(c->keys_d, "", entry, AV_DICT_IGNORE_SUFFIX)) { +av_log(c->fc, AV_LOG_TRACE, + "Read metadata key %s: '%s'\n", entry->key, entry->value); +} + +return 0; +} + static int mov_read_tkhd(MOVContext *c, AVIOContext *pb, MOVAtom atom) { int i; @@ -3786,6 +3846,7 @@ static const MOVParseTableEntry mov_default_parse_table[] = { { MKTAG('h','d','l','r'), mov_read_hdlr }, { MKTAG('i','l','s','t'), mov_read_ilst }, { MKTAG('j','p','2','h'), mov_read_jp2h }, +{ MKTAG('
[FFmpeg-devel] Support of QuickTime metadata atoms
Hi, I'm considering adding support for writing the QuickTime metadata atom structure to support arbitrary key value pairs, similar to those included by ARRI cameras see: https://developer.apple.com/library/prerelease/mac/documentation/QuickTime/QTFF/qtff.pdf p128 To start I was going to try add reading of the metadata, as such I was wondering about FFmpeg's current metadata handling and how to add the metadata found in these atoms to the internal structures. Nominally these metadata atoms can appear in multiple places within the QuickTime file. In particular I wondered if there is any need for name spacing the keys similar to the way the QuickTime specifications use the 'mdta' handler field both as a way to distinguish from the current 'well known named' metadata as well as to potentially support multiple sets of metadata from different parts of the atom tree hierarchy (I haven't worked out if there is a 1:1 mapping with the way ffmpeg currently stores metadata vs potential positions in the atom tree) Has anybody considered this previously and could point me in the right direction? Thanks Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil: Add basic transfer functions for each AVColorTransferCharacteristic
Rather than rewrite history or anything like that people could either apply this on top of my previous patches, or for a view of the whole kit and kaboodle: https://github.com/FFmpeg/FFmpeg/compare/master...KevinJW:image_exr_transfer_characteristics Happy to take further feedback, Kevin On Tue, Sep 1, 2015 at 1:03 PM, wm4 wrote: > On Tue, 1 Sep 2015 12:55:49 +0100 > Kevin Wheatley wrote: > >> On Tue, Sep 1, 2015 at 12:49 PM, wm4 wrote: >> > Identifiers starting with _ are reserved by the system on file scope. >> >> oh yes, switching between different programming languages never a good >> thing for my brain :-) >> >> Would the ffmpeg style be prefixing them up with anything, or just >> leaving off the underscore? > > Yes. For static functions, any identifier should be fine; for > identifiers which are not static and are needed in other files, > but are still library-private, we use a ff_ prefix. > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel From 313d6f147b1beb05ab5676d8cd0a7c745c60051d Mon Sep 17 00:00:00 2001 From: Kevin Wheatley Date: Tue, 1 Sep 2015 14:02:31 +0100 Subject: [PATCH 8/8] avoid the use of reserved names --- libavutil/color_utils.c | 48 +++--- 1 files changed, 24 insertions(+), 24 deletions(-) diff --git a/libavutil/color_utils.c b/libavutil/color_utils.c index f84b5f5..81ba447 100644 --- a/libavutil/color_utils.c +++ b/libavutil/color_utils.c @@ -57,7 +57,7 @@ double avpriv_get_gamma_from_trc(enum AVColorTransferCharacteristic trc) #define BT709_alpha 1.099296826809442 #define BT709_beta 0.018053968510807 -static double _trc_bt709(double Lc) +static double avpriv_trc_bt709(double Lc) { const double a = BT709_alpha; const double b = BT709_beta; @@ -67,17 +67,17 @@ static double _trc_bt709(double Lc) : a * pow(Lc, 0.45) - (a - 1.0); } -static double _trc_gamma22(double Lc) +static double avpriv_trc_gamma22(double Lc) { return (0.0 > Lc) ? 0.0 : pow(Lc, 1.0/ 2.2); } -static double _trc_gamma28(double Lc) +static double avpriv_trc_gamma28(double Lc) { return (0.0 > Lc) ? 0.0 : pow(Lc, 1.0/ 2.8); } -static double _trc_smpte240M(double Lc) +static double avpriv_trc_smpte240M(double Lc) { const double a = 1.1115; const double b = 0.0228; @@ -87,23 +87,23 @@ static double _trc_smpte240M(double Lc) : a * pow(Lc, 0.45) - (a - 1.0); } -static double _trc_linear(double Lc) +static double avpriv_trc_linear(double Lc) { return Lc; } -static double _trc_log(double Lc) +static double avpriv_trc_log(double Lc) { return (0.01 > Lc) ? 0.0 : 1.0 + log10(Lc) / 2.0; } -static double _trc_log_sqrt(double Lc) +static double avpriv_trc_log_sqrt(double Lc) { // sqrt(10) / 1000 return (0.00316227766 > Lc) ? 0.0 : 1.0 + log10(Lc) / 2.5; } -static double _trc_iec61966_2_4(double Lc) +static double avpriv_trc_iec61966_2_4(double Lc) { const double a = BT709_alpha; const double b = BT709_beta; @@ -113,7 +113,7 @@ static double _trc_iec61966_2_4(double Lc) : a * pow( Lc, 0.45) - (a - 1.0); } -static double _trc_bt1361(double Lc) +static double avpriv_trc_bt1361(double Lc) { const double a = BT709_alpha; const double b = BT709_beta; @@ -123,7 +123,7 @@ static double _trc_bt1361(double Lc) : a * pow( Lc, 0.45) - (a - 1.0); } -static double _trc_iec61966_2_1(double Lc) +static double avpriv_trc_iec61966_2_1(double Lc) { const double a = 1.055; const double b = 0.0031308; @@ -133,7 +133,7 @@ static double _trc_iec61966_2_1(double Lc) : a * pow(Lc, 1.0 / 2.4) - (a - 1.0); } -static double _trc_smpte_st2084(double Lc) +static double avpriv_trc_smpte_st2084(double Lc) { const double c1 = 3424.0 / 4096.0; // c3-c2 + 1 const double c2 = 32.0 * 2413.0 / 4096.0; @@ -148,7 +148,7 @@ static double _trc_smpte_st2084(double Lc) } -static double _trc_smpte_st428_1(double Lc) +static double avpriv_trc_smpte_st428_1(double Lc) { return (0.0 > Lc) ? 0.0 : pow(48.0 * Lc / 52.37, 1.0 / 2.6); @@ -162,50 +162,50 @@ avpriv_trc_function avpriv_get_trc_function_from_trc(enum AVColorTransferCharact case AVCOL_TRC_SMPTE170M: case AVCOL_TRC_BT2020_10: case AVCOL_TRC_BT2020_12: -func = _trc_bt709; +func = avpriv_trc_bt709; break; case AVCOL_TRC_GAMMA22: -func = _trc_gamma22; +func = avpriv_trc_gamma22; break; case AVCOL_TRC_GAMMA28: -func = _trc_gamma28; +func = avpriv_trc_gamma28; break; case AVCOL_TRC_SMPTE240M: -f
Re: [FFmpeg-devel] [PATCH] avutil: Add basic transfer functions for each AVColorTransferCharacteristic
On Tue, Sep 1, 2015 at 12:49 PM, wm4 wrote: > Identifiers starting with _ are reserved by the system on file scope. oh yes, switching between different programming languages never a good thing for my brain :-) Would the ffmpeg style be prefixing them up with anything, or just leaving off the underscore? Thanks Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/exr/c Add support for applying a transfer characteristic curve to OpenEXR inputs.
This actually marks up the buffers as having the state given by the applied trc, Kevin On Tue, Sep 1, 2015 at 12:04 PM, Kevin Wheatley wrote: > This adds the actual usage and allows for command lines similar to this: > > ffmpeg -apply_trc bt709 -i linear.exr bt709.png > ffmpeg -apply_trc smpte2084 -i linear.exr smpte2084.png > ffmpeg -apply_trc iec61966_2_1 -i linear.exr sRGB.png > > > Which assuming the correct primaries in the linear OpenEXR file will > generate the appropriate file. > > Kevin From 0e6bd2437146dafef0e6a89c9db378ba534211bd Mon Sep 17 00:00:00 2001 From: Kevin Wheatley Date: Tue, 1 Sep 2015 12:35:55 +0100 Subject: [PATCH 7/7] Mark up the decoded buffer as the appropriate transfer characteristic when applying one --- libavcodec/exr.c |3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/libavcodec/exr.c b/libavcodec/exr.c index 8794da5..92f528a 100644 --- a/libavcodec/exr.c +++ b/libavcodec/exr.c @@ -1308,6 +1308,9 @@ static int decode_frame(AVCodecContext *avctx, void *data, av_log(avctx, AV_LOG_ERROR, "Missing channel list.\n"); return AVERROR_INVALIDDATA; } + +if (s->apply_trc_type != AVCOL_TRC_UNSPECIFIED) +avctx->color_trc = s->apply_trc_type; switch (s->compression) { case EXR_RAW: -- 1.7.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil: Add additional primaries and transfer characteristic enumerations from ITU-T Rec H.265
I noticed that the output from actually tagging buffers with the new states needs patching as well, is there any other place I need to update? Kevin On Tue, Sep 1, 2015 at 9:39 AM, Kevin Wheatley wrote: > As a starting point for my work, I'm wondering about the naming of > these options in the following patch to add the additional transfer > characteristics from H.265. > > I'm not entirely happy with the names as they read a little > ambiguously SMPTE ST vs SMP TEST, obviously I could add another > underscore... > > Thoughts > > Kevin From 403a209cfe8ebee7caca35b678f8d529e8ae5390 Mon Sep 17 00:00:00 2001 From: Kevin Wheatley Date: Tue, 1 Sep 2015 12:34:34 +0100 Subject: [PATCH 6/7] Add SMPTE ST 2084 and ST 428-1 pixel descriptions --- libavutil/pixdesc.c |3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/libavutil/pixdesc.c b/libavutil/pixdesc.c index eb52113..4ef5d83 100644 --- a/libavutil/pixdesc.c +++ b/libavutil/pixdesc.c @@ -2013,13 +2013,14 @@ static const char *color_range_names[AVCOL_RANGE_NB] = { static const char *color_primaries_names[AVCOL_PRI_NB] = { "reserved", "bt709", "unknown", "reserved", "bt470m", "bt470bg", "smpte170m", "smpte240m", "film", "bt2020", +"smpte428_1", }; static const char *color_transfer_names[AVCOL_TRC_NB] = { "reserved", "bt709", "unknown", "reserved", "bt470m", "bt470bg", "smpte170m", "smpte240m", "linear", "log100", "log316", "iec61966-2-4", "bt1361e", "iec61966-2-1", -"bt2020-10", "bt2020-20", +"bt2020-10", "bt2020-20", "smpte2084", "smpte428-1", }; static const char *color_space_names[AVCOL_SPC_NB] = { -- 1.7.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil: Add basic transfer functions for each AVColorTransferCharacteristic
On Tue, Sep 1, 2015 at 12:33 PM, Ronald S. Bultje wrote: > Hi, > > On Tue, Sep 1, 2015 at 7:02 AM, Kevin Wheatley > wrote: > >> Following on from my previous email, this adds some functions to >> actually convert from linear to non-linear encoding. I have another >> that changes the OpenEXR codec to actually use these. > > > Won't performance be crumblingly slow with this kind of API? It seems to > make more sense to write a filter that can convert between different TRC > types, or do it in swscale, and then add some fast implementations > (LUT-based, or approximation-based) alongside the slow versions. I'm using the functions to construct a LUT in the OpenEXR reader when possible, the cleanest design would be to pass floating point into the swscale engine for sure, but I'm not sure I'd personally want to go changing that code. I'm happy for some other approach to be put forward, I have some users who wanted the feature, so I have something that works for their needs so my urgency is minimal. I've performed a few more tests and spotted something missing in one of my earlier changes in the pixel description code, patch to follow... Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avcodec/exr/c Add support for applying a transfer characteristic curve to OpenEXR inputs.
This adds the actual usage and allows for command lines similar to this: ffmpeg -apply_trc bt709 -i linear.exr bt709.png ffmpeg -apply_trc smpte2084 -i linear.exr smpte2084.png ffmpeg -apply_trc iec61966_2_1 -i linear.exr sRGB.png Which assuming the correct primaries in the linear OpenEXR file will generate the appropriate file. Kevin From 25aedae9dc873abcba3a6db3dff503c64cd9e066 Mon Sep 17 00:00:00 2001 From: Kevin Wheatley Date: Tue, 1 Sep 2015 11:02:53 +0100 Subject: [PATCH 5/5] Add support for applying a transfer characteristic curve to OpenEXR inputs. Signed-off-by: Kevin Wheatley --- libavcodec/exr.c | 123 ++ 1 files changed, 96 insertions(+), 27 deletions(-) diff --git a/libavcodec/exr.c b/libavcodec/exr.c index b9de7c1..8794da5 100644 --- a/libavcodec/exr.c +++ b/libavcodec/exr.c @@ -37,6 +37,7 @@ #include "libavutil/imgutils.h" #include "libavutil/intfloat.h" #include "libavutil/opt.h" +#include "libavutil/color_utils.h" #include "avcodec.h" #include "bytestream.h" @@ -110,6 +111,7 @@ typedef struct EXRContext { const char *layer; +enum AVColorTransferCharacteristic apply_trc_type; float gamma; uint16_t gamma_table[65536]; } EXRContext; @@ -842,6 +844,7 @@ static int decode_block(AVCodecContext *avctx, void *tdata, int bxmin = s->xmin * 2 * s->desc->nb_components; int i, x, buf_size = s->buf_size; float one_gamma = 1.0f / s->gamma; +avpriv_trc_function trc_func = avpriv_get_trc_function_from_trc(s->apply_trc_type); int ret; line_offset = AV_RL64(s->gb.buffer + jobnr * 8); @@ -921,24 +924,43 @@ static int decode_block(AVCodecContext *avctx, void *tdata, ptr_x += s->xmin * s->desc->nb_components; if (s->pixel_type == EXR_FLOAT) { // 32-bit -for (x = 0; x < xdelta; x++) { -union av_intfloat32 t; -t.i = bytestream_get_le32(&r); -if (t.f > 0.0f) /* avoid negative values */ -t.f = powf(t.f, one_gamma); -*ptr_x++ = exr_flt2uint(t.i); - -t.i = bytestream_get_le32(&g); -if (t.f > 0.0f) -t.f = powf(t.f, one_gamma); -*ptr_x++ = exr_flt2uint(t.i); - -t.i = bytestream_get_le32(&b); -if (t.f > 0.0f) -t.f = powf(t.f, one_gamma); -*ptr_x++ = exr_flt2uint(t.i); -if (channel_buffer[3]) -*ptr_x++ = exr_flt2uint(bytestream_get_le32(&a)); +if (trc_func) { +for (x = 0; x < xdelta; x++) { +union av_intfloat32 t; +t.i = bytestream_get_le32(&r); +t.f = trc_func(t.f); +*ptr_x++ = exr_flt2uint(t.i); + +t.i = bytestream_get_le32(&g); +t.f = trc_func(t.f); +*ptr_x++ = exr_flt2uint(t.i); + +t.i = bytestream_get_le32(&b); +t.f = trc_func(t.f); +*ptr_x++ = exr_flt2uint(t.i); +if (channel_buffer[3]) +*ptr_x++ = exr_flt2uint(bytestream_get_le32(&a)); +} +} else { +for (x = 0; x < xdelta; x++) { +union av_intfloat32 t; +t.i = bytestream_get_le32(&r); +if (t.f > 0.0f) /* avoid negative values */ +t.f = powf(t.f, one_gamma); +*ptr_x++ = exr_flt2uint(t.i); + +t.i = bytestream_get_le32(&g); +if (t.f > 0.0f) +t.f = powf(t.f, one_gamma); +*ptr_x++ = exr_flt2uint(t.i); + +t.i = bytestream_get_le32(&b); +if (t.f > 0.0f) +t.f = powf(t.f, one_gamma); +*ptr_x++ = exr_flt2uint(t.i); +if (channel_buffer[3]) +*ptr_x++ = exr_flt2uint(bytestream_get_le32(&a)); +} } } else { // 16-bit @@ -1364,21 +1386,31 @@ static av_cold int decode_init(AVCodecContext *avctx) uint32_t i; union av_intfloat32 t; float one_gamma = 1.0f / s->gamma; +avpriv_trc_function trc_func = NULL; s->avctx = avctx; -if (one_gamma > 0.f && one_gamma < 1.0001f) { -for (i = 0; i < 65536; ++i) -s->gamma_table[i] = exr_halflt2uint(i); -} else { +trc_func = avpriv_get_trc_function_from_trc(s->apply_trc_type); +if (trc_func) { for (i = 0; i < 65536; ++i) { t = exr_half2float(i); -/*
[FFmpeg-devel] [PATCH] avutil: Add basic transfer functions for each AVColorTransferCharacteristic
Following on from my previous email, this adds some functions to actually convert from linear to non-linear encoding. I have another that changes the OpenEXR codec to actually use these. Kevin From 59299c23489fadeb11330b51f83b152194f0810e Mon Sep 17 00:00:00 2001 From: Kevin Wheatley Date: Tue, 1 Sep 2015 11:41:38 +0100 Subject: [PATCH 4/5] Add basic transfer functions for each AVColorTransferCharacteristic Most functions are valid over a domain and range of [0.0-1.0] but some are defined over greater. This patch does not deal with AVColorRange and assumes AVCOL_RANGE_JPEG for the returned values. Signed-off-by: Kevin Wheatley --- libavutil/color_utils.c | 166 +++ libavutil/color_utils.h | 17 + 2 files changed, 183 insertions(+), 0 deletions(-) diff --git a/libavutil/color_utils.c b/libavutil/color_utils.c index 59146be..f84b5f5 100644 --- a/libavutil/color_utils.c +++ b/libavutil/color_utils.c @@ -18,6 +18,9 @@ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA */ +#include +#include + #include "libavutil/color_utils.h" #include "libavutil/pixfmt.h" @@ -50,3 +53,166 @@ double avpriv_get_gamma_from_trc(enum AVColorTransferCharacteristic trc) } return gamma; } + +#define BT709_alpha 1.099296826809442 +#define BT709_beta 0.018053968510807 + +static double _trc_bt709(double Lc) +{ +const double a = BT709_alpha; +const double b = BT709_beta; + +return (0.0 > Lc) ? 0.0 + : ( b > Lc) ? 4.500 * Lc + : a * pow(Lc, 0.45) - (a - 1.0); +} + +static double _trc_gamma22(double Lc) +{ +return (0.0 > Lc) ? 0.0 : pow(Lc, 1.0/ 2.2); +} + +static double _trc_gamma28(double Lc) +{ +return (0.0 > Lc) ? 0.0 : pow(Lc, 1.0/ 2.8); +} + +static double _trc_smpte240M(double Lc) +{ +const double a = 1.1115; +const double b = 0.0228; + +return (0.0 > Lc) ? 0.0 + : ( b > Lc) ? 4.000 * Lc + : a * pow(Lc, 0.45) - (a - 1.0); +} + +static double _trc_linear(double Lc) +{ +return Lc; +} + +static double _trc_log(double Lc) +{ +return (0.01 > Lc) ? 0.0 : 1.0 + log10(Lc) / 2.0; +} + +static double _trc_log_sqrt(double Lc) +{ +// sqrt(10) / 1000 +return (0.00316227766 > Lc) ? 0.0 : 1.0 + log10(Lc) / 2.5; +} + +static double _trc_iec61966_2_4(double Lc) +{ +const double a = BT709_alpha; +const double b = BT709_beta; + +return (-b >= Lc) ? -a * pow(-Lc, 0.45) + (a - 1.0) + : ( b > Lc) ? 4.500 * Lc + : a * pow( Lc, 0.45) - (a - 1.0); +} + +static double _trc_bt1361(double Lc) +{ +const double a = BT709_alpha; +const double b = BT709_beta; + +return (-0.0045 >= Lc) ? -(a * pow(-4.0 * Lc, 0.45) + (a - 1.0)) / 4.0 + : ( b > Lc) ? 4.500 * Lc + : a * pow( Lc, 0.45) - (a - 1.0); +} + +static double _trc_iec61966_2_1(double Lc) +{ +const double a = 1.055; +const double b = 0.0031308; + +return (0.0 > Lc) ? 0.0 + : ( b > Lc) ? 12.92 * Lc + : a * pow(Lc, 1.0 / 2.4) - (a - 1.0); +} + +static double _trc_smpte_st2084(double Lc) +{ +const double c1 = 3424.0 / 4096.0; // c3-c2 + 1 +const double c2 = 32.0 * 2413.0 / 4096.0; +const double c3 = 32.0 * 2392.0 / 4096.0; +const double m = 128.0 * 2523.0 / 4096.0; +const double n = 0.25 * 2610.0 / 4096.0; +const double L = Lc / 1.0; +const double Ln = pow(L, n); + +return (0.0 > Lc) ? 0.0 + : pow((c1 + c2 * Ln) / (1.0 + c3 * Ln), m); + +} + +static double _trc_smpte_st428_1(double Lc) +{ +return (0.0 > Lc) ? 0.0 + : pow(48.0 * Lc / 52.37, 1.0 / 2.6); +} + +avpriv_trc_function avpriv_get_trc_function_from_trc(enum AVColorTransferCharacteristic trc) +{ +avpriv_trc_function func = NULL; +switch (trc) { +case AVCOL_TRC_BT709: +case AVCOL_TRC_SMPTE170M: +case AVCOL_TRC_BT2020_10: +case AVCOL_TRC_BT2020_12: +func = _trc_bt709; +break; + +case AVCOL_TRC_GAMMA22: +func = _trc_gamma22; +break; +case AVCOL_TRC_GAMMA28: +func = _trc_gamma28; +break; + +case AVCOL_TRC_SMPTE240M: +func = _trc_smpte240M; +break; + +case AVCOL_TRC_LINEAR: +func = _trc_linear; +break; + +case AVCOL_TRC_LOG: +func = _trc_log; +break; + +case AVCOL_TRC_LOG_SQRT: +func = _trc_log_sqrt; +break; + +case AVCOL_TRC_IEC61966_2_4: +func = _trc_iec61966_2_4; +break; + +case AVCOL_TRC_BT1361_ECG: +func = _trc_bt1361; +break; + +case AVCOL_TRC_IEC61966_2_1: +
[FFmpeg-devel] [PATCH] avutil: Add additional primaries and transfer characteristic enumerations from ITU-T Rec H.265
As a starting point for my work, I'm wondering about the naming of these options in the following patch to add the additional transfer characteristics from H.265. I'm not entirely happy with the names as they read a little ambiguously SMPTE ST vs SMP TEST, obviously I could add another underscore... Thoughts Kevin From 6190c0ee42fd2cb4ea1370111b46e03ea55459af Mon Sep 17 00:00:00 2001 From: Kevin Wheatley Date: Fri, 28 Aug 2015 15:20:22 +0100 Subject: [PATCH] Add additional primaries and transfer characteristic enumerations from ITU-T Rec H.265 Signed-off-by: Kevin Wheatley --- libavutil/pixfmt.h | 21 - 1 files changed, 12 insertions(+), 9 deletions(-) diff --git a/libavutil/pixfmt.h b/libavutil/pixfmt.h index 029c911..0f93050 100644 --- a/libavutil/pixfmt.h +++ b/libavutil/pixfmt.h @@ -475,17 +475,18 @@ enum AVPixelFormat { */ enum AVColorPrimaries { AVCOL_PRI_RESERVED0 = 0, -AVCOL_PRI_BT709 = 1, ///< also ITU-R BT1361 / IEC 61966-2-4 / SMPTE RP177 Annex B +AVCOL_PRI_BT709 = 1, ///< also ITU-R BT1361 / IEC 61966-2-4 / SMPTE RP177 Annex B AVCOL_PRI_UNSPECIFIED = 2, AVCOL_PRI_RESERVED= 3, -AVCOL_PRI_BT470M = 4, ///< also FCC Title 47 Code of Federal Regulations 73.682 (a)(20) - -AVCOL_PRI_BT470BG = 5, ///< also ITU-R BT601-6 625 / ITU-R BT1358 625 / ITU-R BT1700 625 PAL & SECAM -AVCOL_PRI_SMPTE170M = 6, ///< also ITU-R BT601-6 525 / ITU-R BT1358 525 / ITU-R BT1700 NTSC -AVCOL_PRI_SMPTE240M = 7, ///< functionally identical to above -AVCOL_PRI_FILM= 8, ///< colour filters using Illuminant C -AVCOL_PRI_BT2020 = 9, ///< ITU-R BT2020 -AVCOL_PRI_NB, ///< Not part of ABI +AVCOL_PRI_BT470M = 4, ///< also FCC Title 47 Code of Federal Regulations 73.682 (a)(20) + +AVCOL_PRI_BT470BG = 5, ///< also ITU-R BT601-6 625 / ITU-R BT1358 625 / ITU-R BT1700 625 PAL & SECAM +AVCOL_PRI_SMPTE170M = 6, ///< also ITU-R BT601-6 525 / ITU-R BT1358 525 / ITU-R BT1700 NTSC +AVCOL_PRI_SMPTE240M = 7, ///< functionally identical to above +AVCOL_PRI_FILM= 8, ///< colour filters using Illuminant C +AVCOL_PRI_BT2020 = 9, ///< ITU-R BT2020 +AVCOL_PRI_SMPTE428_1 = 10, ///< SMPTE ST 428-1 (CIE 1931 XYZ) +AVCOL_PRI_NB, ///< Not part of ABI }; /** @@ -508,6 +509,8 @@ enum AVColorTransferCharacteristic { AVCOL_TRC_IEC61966_2_1 = 13, ///< IEC 61966-2-1 (sRGB or sYCC) AVCOL_TRC_BT2020_10= 14, ///< ITU-R BT2020 for 10 bit system AVCOL_TRC_BT2020_12= 15, ///< ITU-R BT2020 for 12 bit system +AVCOL_TRC_SMPTEST2084 = 16, ///< SMPTE ST 2084 for 10, 12, 14 and 16 bit systems +AVCOL_TRC_SMPTEST428_1 = 17, ///< SMPTE ST 428-1 AVCOL_TRC_NB,///< Not part of ABI }; -- 1.7.1 From b11126e0e80589d0ac6e12b301d846cd90b9caef Mon Sep 17 00:00:00 2001 From: Kevin Wheatley Date: Tue, 1 Sep 2015 09:14:01 +0100 Subject: [PATCH 2/2] Add CLI options for SMPTE ST 2084 and ST 428-1 transfer characteristics Signed-off-by: Kevin Wheatley --- libavcodec/options_table.h |2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/libavcodec/options_table.h b/libavcodec/options_table.h index 93d4f77..f698262 100644 --- a/libavcodec/options_table.h +++ b/libavcodec/options_table.h @@ -444,6 +444,8 @@ static const AVOption avcodec_options[] = { {"iec61966_2_1", "IEC 61966-2-1",0, AV_OPT_TYPE_CONST, {.i64 = AVCOL_TRC_IEC61966_2_1 }, INT_MIN, INT_MAX, V|E|D, "color_trc_type"}, {"bt2020_10bit", "BT.2020 - 10 bit", 0, AV_OPT_TYPE_CONST, {.i64 = AVCOL_TRC_BT2020_10 },INT_MIN, INT_MAX, V|E|D, "color_trc_type"}, {"bt2020_12bit", "BT.2020 - 12 bit", 0, AV_OPT_TYPE_CONST, {.i64 = AVCOL_TRC_BT2020_12 },INT_MIN, INT_MAX, V|E|D, "color_trc_type"}, +{"smpte2084","SMPTE ST 2084",0, AV_OPT_TYPE_CONST, {.i64 = AVCOL_TRC_SMPTEST2084 }, INT_MIN, INT_MAX, V|E|D, "color_trc_type"}, +{"smpte428_1", "SMPTE ST 428-1", 0, AV_OPT_TYPE_CONST, {.i64 = AVCOL_TRC_SMPTEST428_1 }, INT_MIN, INT_MAX, V|E|D, "color_trc_type"}, {"colorspace", "color space", OFFSET(colorspace), AV_OPT_TYPE_INT, {.i64 = AVCOL_SPC_UNSPECIFIED }, 0, AVCOL_SPC_NB-1, V|E|D, "colorspace_type"}, {"rgb", "RGB", 0, AV_OPT_TYPE_CONST, {.i64 = AVCOL_SPC_RGB }, INT_MIN, INT_MAX, V|E|D, "colorspace_type"}, {"bt709", "BT.709", 0, AV_OPT_TYPE_CONST, {.i64 = AVCOL_SPC_BT709 }, INT_MIN, INT_MAX, V|E|D, "colorspace_type"}, -- 1.7.1 From 7bd290695c352bacc60d659e688f1530c441717d Mon Sep 17 00:00:00 2001 From: Kevin Wheatley Date: Tue, 1 Sep 2015 09:32:42 +0
Re: [FFmpeg-devel] Enhancing/extending OpenEXR 'gamma' feature
On Fri, Aug 28, 2015 at 3:54 PM, Paul B Mahol wrote: > On 8/28/15, Kevin Wheatley wrote: > Feel free to do it. > > Note however that those various curves really belong to avfilter once > swscale supports float pixel format and exr decoder make use of it. Absolutely, most of the tools I normally work with are full float/double internally, I was going to start with the functions that perform the non-linear encoding, but if I'm to consider the option they will need to work in the swscale code it opens a few questions: Should I provide functions going too and from linear? This would allow them to be composed to perform cross conversion. float or double precision? - if these functions will be composed together is there a need for double precision? Thanks Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] Enhancing/extending OpenEXR 'gamma' feature
HI, I was wondering what others think about the option of extending the OpenEXR reader's gamma option to also include various transfer characteristic curves, potentially corresponding to the various AVCOL_TRC_* options, though I don't immediately have a need for all of them. In addition I could see the need for other curves like those of cinema camera vendor log curves, for instance Red, Sony, ARRI. From a user point of view I'd see a command line option specific to the EXR codec that would take a subset of the colour_trc options (bt709, etc) extended to include some camera transfer curves and it would apply the appropriate function prior to the encoding as AV_PIX_FMT_RGB48/64 as is done currently. I'm thinking the command line option would be called 'apply_trc' or something similar. Why it is needed is simply that the gamma function isn't really sufficient for matching all the typical formats we see in production use. What would be nice would be to embed more colour adjustments prior to the truncation to integer values but that could be quite complex and may be better handled outside of ffmpeg. I've seen that there was a mention of something similar in the list archive, but could not find anything other than those emails. I'd be great full if any of the devs could comment on the idea before I start on the implementation. Thanks Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]Remove framerates from dnxhd encoder help
Though not definitive in terms of a specification: http://www.avid.com/static/resources/us/documents/dnxhd.pdf pages 9/10 has the table of options. Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [GSoC] Patch which adds support for gamma correct scaling
I'll add that for some encodings filters with negative lobes will cause ringing no matter the direction up/down, especially true for higher dynamic range encodings like log and HDR options like SMPTE ST 2084 Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [GSoC] Patch which adds support for gamma correct scaling
Pedro I understand the desire to 'degamma' images prior to scaling, I'd be interested in understanding how this could work with the colour_trc options rather than only assuming a simple gamma. Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] optionally write QuickTime gama atom (was Re: Gamma function)
Michael, wm4, Thank you for your feedback, I believe I've addressed your concerns, please let me know if there is anything else that needs fixing. Kevin From 079ff77a1885b5ef879a8cd3b4c032a3182e8e67 Mon Sep 17 00:00:00 2001 From: Kevin Wheatley Date: Thu, 5 Mar 2015 10:37:51 + Subject: [PATCH 1/2] Extract gamma determination from PNG encoder for future use. Adds private avpriv_get_gamma_from_trc() function to libavutil. Signed-off-by: Kevin Wheatley --- libavcodec/pngenc.c | 29 +++-- libavutil/Makefile |1 + libavutil/color_utils.c | 52 +++ libavutil/color_utils.h | 39 +++ libavutil/version.h |2 +- 5 files changed, 97 insertions(+), 26 deletions(-) create mode 100644 libavutil/color_utils.c create mode 100644 libavutil/color_utils.h diff --git a/libavcodec/pngenc.c b/libavcodec/pngenc.c index 6c9f43e..9bdefc4 100644 --- a/libavcodec/pngenc.c +++ b/libavcodec/pngenc.c @@ -28,6 +28,7 @@ #include "libavutil/avassert.h" #include "libavutil/libm.h" #include "libavutil/opt.h" +#include "libavutil/color_utils.h" #include @@ -277,31 +278,9 @@ static int png_get_chrm(enum AVColorPrimaries prim, uint8_t *buf) static int png_get_gama(enum AVColorTransferCharacteristic trc, uint8_t *buf) { -double gamma; -switch (trc) { -case AVCOL_TRC_BT709: -case AVCOL_TRC_SMPTE170M: -case AVCOL_TRC_SMPTE240M: -case AVCOL_TRC_BT1361_ECG: -case AVCOL_TRC_BT2020_10: -case AVCOL_TRC_BT2020_12: -/* these share a segmented TRC, but gamma 1.961 is a close - approximation, and also more correct for decoding content */ -gamma = 1.961; -break; -case AVCOL_TRC_GAMMA22: -case AVCOL_TRC_IEC61966_2_1: -gamma = 2.2; -break; -case AVCOL_TRC_GAMMA28: -gamma = 2.8; -break; -case AVCOL_TRC_LINEAR: -gamma = 1.0; -break; -default: -return 0; -} +double gamma = avpriv_get_gamma_from_trc(trc); +if (gamma <= 1e-6) +return 0; AV_WB32_PNG(buf, 1.0 / gamma); return 1; diff --git a/libavutil/Makefile b/libavutil/Makefile index 6caf896..df85cd1 100644 --- a/libavutil/Makefile +++ b/libavutil/Makefile @@ -88,6 +88,7 @@ OBJS = adler32.o\ cast5.o \ camellia.o \ channel_layout.o \ + color_utils.o\ cpu.o\ crc.o\ des.o\ diff --git a/libavutil/color_utils.c b/libavutil/color_utils.c new file mode 100644 index 000..59146be --- /dev/null +++ b/libavutil/color_utils.c @@ -0,0 +1,52 @@ +/* + * Copyright (c) 2015 Kevin Wheatley + * + * This file is part of FFmpeg. + * + * FFmpeg is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * FFmpeg is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with FFmpeg; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#include "libavutil/color_utils.h" +#include "libavutil/pixfmt.h" + +double avpriv_get_gamma_from_trc(enum AVColorTransferCharacteristic trc) +{ +double gamma; +switch (trc) { +case AVCOL_TRC_BT709: +case AVCOL_TRC_SMPTE170M: +case AVCOL_TRC_SMPTE240M: +case AVCOL_TRC_BT1361_ECG: +case AVCOL_TRC_BT2020_10: +case AVCOL_TRC_BT2020_12: +/* these share a segmented TRC, but gamma 1.961 is a close + approximation, and also more correct for decoding content */ +gamma = 1.961; +break; +case AVCOL_TRC_GAMMA22: +case AVCOL_TRC_IEC61966_2_1: +gamma = 2.2; +break; +case AVCOL_TRC_GAMMA28: +gamma = 2.8; +break; +case AVCOL_TRC_LINEAR: +gamma = 1.0; +break; +default: +gamma = 0.0; // Unknown valu
[FFmpeg-devel] [PATCH] optionally write QuickTime gama atom (was Re: Gamma function)
Updated to use lrint() On Mon, Mar 2, 2015 at 12:54 PM, Kevin Wheatley wrote: > Something like this - note this adds no tests, but fate still passes for me. > > Kevin From 794c8c3cf1e8506393fbcb4ddf1360042b0fc82f Mon Sep 17 00:00:00 2001 From: Kevin Wheatley Date: Mon, 2 Mar 2015 12:50:53 + Subject: [PATCH] Add 'gama' atom for .mov only, reuses pngenc.c gamma values by sharing function in new color_utils.[ch] file. Use lrint() instead of round() Signed-off-by: Kevin Wheatley --- libavcodec/pngenc.c | 29 +++-- libavformat/movenc.c| 35 +++ libavformat/movenc.h|2 + libavutil/Makefile |2 + libavutil/color_utils.c | 52 +++ libavutil/color_utils.h | 29 ++ 6 files changed, 124 insertions(+), 25 deletions(-) create mode 100644 libavutil/color_utils.c create mode 100644 libavutil/color_utils.h diff --git a/libavcodec/pngenc.c b/libavcodec/pngenc.c index 4e67ce2..fddcca4 100644 --- a/libavcodec/pngenc.c +++ b/libavcodec/pngenc.c @@ -28,6 +28,7 @@ #include "libavutil/avassert.h" #include "libavutil/libm.h" #include "libavutil/opt.h" +#include "libavutil/color_utils.h" #include @@ -277,31 +278,9 @@ static int png_get_chrm(enum AVColorPrimaries prim, uint8_t *buf) static int png_get_gama(enum AVColorTransferCharacteristic trc, uint8_t *buf) { -double gamma; -switch (trc) { -case AVCOL_TRC_BT709: -case AVCOL_TRC_SMPTE170M: -case AVCOL_TRC_SMPTE240M: -case AVCOL_TRC_BT1361_ECG: -case AVCOL_TRC_BT2020_10: -case AVCOL_TRC_BT2020_12: -/* these share a segmented TRC, but gamma 1.961 is a close - approximation, and also more correct for decoding content */ -gamma = 1.961; -break; -case AVCOL_TRC_GAMMA22: -case AVCOL_TRC_IEC61966_2_1: -gamma = 2.2; -break; -case AVCOL_TRC_GAMMA28: -gamma = 2.8; -break; -case AVCOL_TRC_LINEAR: -gamma = 1.0; -break; -default: -return 0; -} +double gamma = get_gamma_from_trc(trc); +if (gamma <= 1e-6) +return 0; AV_WB32_PNG(buf, 1.0 / gamma); return 1; diff --git a/libavformat/movenc.c b/libavformat/movenc.c index e496ba4..7c10d0e 100644 --- a/libavformat/movenc.c +++ b/libavformat/movenc.c @@ -40,10 +40,12 @@ #include "libavutil/avstring.h" #include "libavutil/intfloat.h" #include "libavutil/mathematics.h" +#include "libavutil/libm.h" #include "libavutil/opt.h" #include "libavutil/dict.h" #include "libavutil/pixdesc.h" #include "libavutil/timecode.h" +#include "libavutil/color_utils.h" #include "hevc.h" #include "rtpenc.h" #include "mov_chan.h" @@ -65,6 +67,7 @@ static const AVOption options[] = { { "frag_discont", "Signal that the next fragment is discontinuous from earlier ones", 0, AV_OPT_TYPE_CONST, {.i64 = FF_MOV_FLAG_FRAG_DISCONT}, INT_MIN, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM, "movflags" }, { "delay_moov", "Delay writing the initial moov until the first fragment is cut, or until the first fragment flush", 0, AV_OPT_TYPE_CONST, {.i64 = FF_MOV_FLAG_DELAY_MOOV}, INT_MIN, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM, "movflags" }, { "write_colr", "Write colr atom", 0, AV_OPT_TYPE_CONST, {.i64 = FF_MOV_FLAG_WRITE_COLR}, INT_MIN, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM, "movflags" }, +{ "write_gama", "Write deprecated gama atom", 0, AV_OPT_TYPE_CONST, {.i64 = FF_MOV_FLAG_WRITE_GAMA}, INT_MIN, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM, "movflags" }, FF_RTP_FLAG_OPTS(MOVMuxContext, rtp_flags), { "skip_iods", "Skip writing iods atom.", offsetof(MOVMuxContext, iods_skip), AV_OPT_TYPE_INT, {.i64 = 1}, 0, 1, AV_OPT_FLAG_ENCODING_PARAM}, { "iods_audio_profile", "iods audio profile atom.", offsetof(MOVMuxContext, iods_audio_profile), AV_OPT_TYPE_INT, {.i64 = -1}, -1, 255, AV_OPT_FLAG_ENCODING_PARAM}, @@ -77,6 +80,7 @@ static const AVOption options[] = { { "brand","Override major brand", offsetof(MOVMuxContext, major_brand), AV_OPT_TYPE_STRING, {.str = NULL}, .flags = AV_OPT_FLAG_ENCODING_PARAM }, { "use_editlist", "use edit list", offsetof(MOVMuxContext, use_editlist), AV_OPT_TYPE_INT, {.i64 = -1}, -1, 1, AV_OPT_FLAG_ENCODING_PARAM}, { "fragment_index", "Fragment number of the next fragment", offsetof(MOVMuxContext, fragments), AV_OPT_TYPE_INT, {.i64 = 0}, 0, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM}, +{ "mov_gamma",
Re: [FFmpeg-devel] Gamma function (was Re: [PATCH] lavc/pngenc: Support writing colorspace tags.)
Something like this - note this adds no tests, but fate still passes for me. Kevin From db02ae26c3c4278da4ed328e767bab98271da51e Mon Sep 17 00:00:00 2001 From: Kevin Wheatley Date: Mon, 2 Mar 2015 12:50:53 + Subject: [PATCH] Add 'gama' atom for .mov only, reuses pngenc.c gamma values by sharing function in new color_utils.[ch] file. Signed-off-by: Kevin Wheatley --- libavcodec/pngenc.c | 29 +++-- libavformat/movenc.c| 34 ++ libavformat/movenc.h|2 + libavutil/Makefile |2 + libavutil/color_utils.c | 52 +++ libavutil/color_utils.h | 29 ++ 6 files changed, 123 insertions(+), 25 deletions(-) create mode 100644 libavutil/color_utils.c create mode 100644 libavutil/color_utils.h diff --git a/libavcodec/pngenc.c b/libavcodec/pngenc.c index 9fd8eef..db849fb 100644 --- a/libavcodec/pngenc.c +++ b/libavcodec/pngenc.c @@ -27,6 +27,7 @@ #include "libavutil/avassert.h" #include "libavutil/opt.h" +#include "libavutil/color_utils.h" #include @@ -276,31 +277,9 @@ static int png_get_chrm(enum AVColorPrimaries prim, uint8_t *buf) static int png_get_gama(enum AVColorTransferCharacteristic trc, uint8_t *buf) { -double gamma; -switch (trc) { -case AVCOL_TRC_BT709: -case AVCOL_TRC_SMPTE170M: -case AVCOL_TRC_SMPTE240M: -case AVCOL_TRC_BT1361_ECG: -case AVCOL_TRC_BT2020_10: -case AVCOL_TRC_BT2020_12: -/* these share a segmented TRC, but gamma 1.961 is a close - approximation, and also more correct for decoding content */ -gamma = 1.961; -break; -case AVCOL_TRC_GAMMA22: -case AVCOL_TRC_IEC61966_2_1: -gamma = 2.2; -break; -case AVCOL_TRC_GAMMA28: -gamma = 2.8; -break; -case AVCOL_TRC_LINEAR: -gamma = 1.0; -break; -default: -return 0; -} +double gamma = get_gamma_from_trc(trc); +if (gamma <= 1e-6) +return 0; AV_WB32_PNG(buf, 1.0 / gamma); return 1; diff --git a/libavformat/movenc.c b/libavformat/movenc.c index e496ba4..05516a8 100644 --- a/libavformat/movenc.c +++ b/libavformat/movenc.c @@ -44,6 +44,7 @@ #include "libavutil/dict.h" #include "libavutil/pixdesc.h" #include "libavutil/timecode.h" +#include "libavutil/color_utils.h" #include "hevc.h" #include "rtpenc.h" #include "mov_chan.h" @@ -65,6 +66,7 @@ static const AVOption options[] = { { "frag_discont", "Signal that the next fragment is discontinuous from earlier ones", 0, AV_OPT_TYPE_CONST, {.i64 = FF_MOV_FLAG_FRAG_DISCONT}, INT_MIN, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM, "movflags" }, { "delay_moov", "Delay writing the initial moov until the first fragment is cut, or until the first fragment flush", 0, AV_OPT_TYPE_CONST, {.i64 = FF_MOV_FLAG_DELAY_MOOV}, INT_MIN, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM, "movflags" }, { "write_colr", "Write colr atom", 0, AV_OPT_TYPE_CONST, {.i64 = FF_MOV_FLAG_WRITE_COLR}, INT_MIN, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM, "movflags" }, +{ "write_gama", "Write deprecated gama atom", 0, AV_OPT_TYPE_CONST, {.i64 = FF_MOV_FLAG_WRITE_GAMA}, INT_MIN, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM, "movflags" }, FF_RTP_FLAG_OPTS(MOVMuxContext, rtp_flags), { "skip_iods", "Skip writing iods atom.", offsetof(MOVMuxContext, iods_skip), AV_OPT_TYPE_INT, {.i64 = 1}, 0, 1, AV_OPT_FLAG_ENCODING_PARAM}, { "iods_audio_profile", "iods audio profile atom.", offsetof(MOVMuxContext, iods_audio_profile), AV_OPT_TYPE_INT, {.i64 = -1}, -1, 255, AV_OPT_FLAG_ENCODING_PARAM}, @@ -77,6 +79,7 @@ static const AVOption options[] = { { "brand","Override major brand", offsetof(MOVMuxContext, major_brand), AV_OPT_TYPE_STRING, {.str = NULL}, .flags = AV_OPT_FLAG_ENCODING_PARAM }, { "use_editlist", "use edit list", offsetof(MOVMuxContext, use_editlist), AV_OPT_TYPE_INT, {.i64 = -1}, -1, 1, AV_OPT_FLAG_ENCODING_PARAM}, { "fragment_index", "Fragment number of the next fragment", offsetof(MOVMuxContext, fragments), AV_OPT_TYPE_INT, {.i64 = 0}, 0, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM}, +{ "mov_gamma", "gamma value for gama atom", offsetof(MOVMuxContext, gamma), AV_OPT_TYPE_FLOAT, {.dbl = 0.0 }, 0.0, 10, AV_OPT_FLAG_ENCODING_PARAM}, { NULL }, }; @@ -1519,6 +1522,31 @@ static int mov_write_pasp_tag(AVIOContext *pb, MOVTrack *track) return 16; } +static int mov_write_gama_tag(AVIOContext *pb, MOVTrack *track, double gamma) +{ +uint32_t gama =
Re: [FFmpeg-devel] Gamma function (was Re: [PATCH] lavc/pngenc: Support writing colorspace tags.)
On Mon, Mar 2, 2015 at 12:17 PM, Michael Niedermayer wrote: > the various colorspace options should pass from decoder over the > video filter chain to the encoder and then muxer > its best to change them from the command line through a video > filter, this also ensures that the value reaching the encoder and > muxer matches I think I understand what you are suggesting in general, but in this case this is an old metadata option that I don't imagine any other format would want to propagate, it existed prior to the colr/nclc/nclx and is mostly superceded by that. I say mostly because the trc options don't allow for arbitrary values like '2.6' for instance, and we've also come across apps that need the gama atom setting to a value and the trc left as 'unspecified' in order to correctly decode. even when "rec709" would have been the correct value. That was why I was thinking it should be a .mov muxer only option. Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] Gamma function (was Re: [PATCH] lavc/pngenc: Support writing colorspace tags.)
On Sat, Feb 28, 2015 at 1:50 AM, Niklas Haas wrote: > +static int png_get_gama(enum AVColorTransferCharacteristic trc, uint8_t *buf) > +{ > +double gamma; > +switch (trc) { > +case AVCOL_TRC_BT709: > +case AVCOL_TRC_SMPTE170M: > +case AVCOL_TRC_SMPTE240M: > +case AVCOL_TRC_BT1361_ECG: > +case AVCOL_TRC_BT2020_10: > +case AVCOL_TRC_BT2020_12: > +/* these share a segmented TRC, but gamma 1.961 is a close > + approximation, and also more correct for decoding content */ > +gamma = 1.961; > +break; > +case AVCOL_TRC_GAMMA22: > +case AVCOL_TRC_IEC61966_2_1: > +gamma = 2.2; > +break; > +case AVCOL_TRC_GAMMA28: > +gamma = 2.8; > +break; > +case AVCOL_TRC_LINEAR: > +gamma = 1.0; > +break; > +default: > +return 0; > +} > + > +AV_WB32_PNG(buf, 1.0 / gamma); > +return 1; > +} > + Co-incidentally I was thinking of submitting a patch to write the old QuickTime gama atom and most of this function would be useful if extracted to a common place (everything but the AV_WB32_PNG() as .mov's need a different scaling). Where should it go? libavutil? As part of it I would also allow manually specifying a specific value to encode on the command line, but I wondered about how to form the command line options... Case 1 User wants to write gamma based on TRC settings, using the above (now extracted) function. +write_gama on movflags? Case 2 User wants to specify the actual gamma on the command line Same as above, plus a -gamma I'd suggest to require +write_gama on the command line as the gama atom is deprecated and should really be only needed for backwards compatibity/broken tools. Comments? Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] Gaining access to a encoder context in avformat?
On Fri, Feb 20, 2015 at 5:51 PM, Michael Niedermayer wrote: > theres some code that memcpies extradata into vos_data and that is > skiped if TAG_IS_AVCI(trk->tag), try to also skip this for DNxHD Michael, thanks for the pointer, there are actually two points in the code that appear to need the guard against overwriting, I'm attaching an updated patch for comments. Kevin From bd5543cd6a227e64e66eb5ac909e5efeddfeb3a8 Mon Sep 17 00:00:00 2001 From: Kevin Wheatley Date: Tue, 24 Feb 2015 10:00:07 + Subject: [PATCH] Using the copy codec ACLR atoms are incorrectly written - fix. During the creation of the ACLR atom we are assuming the vos_data contains the DNxHD header. This change makes this explicit and ensures we don't over write the stream with the extra_data. Signed-off-by: Kevin Wheatley --- libavformat/movenc.c | 31 +++ 1 files changed, 27 insertions(+), 4 deletions(-) diff --git a/libavformat/movenc.c b/libavformat/movenc.c index 210f78e..276b711 100644 --- a/libavformat/movenc.c +++ b/libavformat/movenc.c @@ -1033,6 +1033,27 @@ static int mov_write_hvcc_tag(AVIOContext *pb, MOVTrack *track) static int mov_write_avid_tag(AVIOContext *pb, MOVTrack *track) { int i; +int interlaced; +int cid; + +if (track->vos_data && track->vos_len > 0x29) { +if (track->vos_data[0] == 0x00 && +track->vos_data[1] == 0x00 && +track->vos_data[2] == 0x02 && +track->vos_data[3] == 0x80 && +(track->vos_data[4] == 0x01 || track->vos_data[4] == 0x02)) { +/* looks like a DNxHD bit stream */ +interlaced = (track->vos_data[5] & 2); +cid = AV_RB32(track->vos_data + 0x28); +} else { +av_log(NULL, AV_LOG_WARNING, "Could not locate DNxHD bit stream in vos_data\n"); +return 0; +} +} else { +av_log(NULL, AV_LOG_WARNING, "Could not locate DNxHD bit stream, vos_data too small\n"); +return 0; +} + avio_wb32(pb, 24); /* size */ ffio_wfourcc(pb, "ACLR"); ffio_wfourcc(pb, "ACLR"); @@ -1056,10 +1077,10 @@ static int mov_write_avid_tag(AVIOContext *pb, MOVTrack *track) ffio_wfourcc(pb, "ARES"); ffio_wfourcc(pb, "ARES"); ffio_wfourcc(pb, "0001"); -avio_wb32(pb, AV_RB32(track->vos_data + 0x28)); /* dnxhd cid, some id ? */ +avio_wb32(pb, cid); /* dnxhd cid, some id ? */ avio_wb32(pb, track->enc->width); /* values below are based on samples created with quicktime and avid codecs */ -if (track->vos_data[5] & 2) { // interlaced +if (interlaced) { avio_wb32(pb, track->enc->height / 2); avio_wb32(pb, 2); /* unknown */ avio_wb32(pb, 0); /* unknown */ @@ -4165,7 +4186,9 @@ int ff_mov_write_packet(AVFormatContext *s, AVPacket *pkt) samples_in_chunk = 1; /* copy extradata if it exists */ -if (trk->vos_len == 0 && enc->extradata_size > 0 && !TAG_IS_AVCI(trk->tag)) { +if (trk->vos_len == 0 && enc->extradata_size > 0 && +!TAG_IS_AVCI(trk->tag) && +(enc->codec_id != AV_CODEC_ID_DNXHD)) { trk->vos_len = enc->extradata_size; trk->vos_data = av_malloc(trk->vos_len); if (!trk->vos_data) { @@ -4952,7 +4975,7 @@ static int mov_write_header(AVFormatContext *s) if (st->codec->extradata_size) { if (st->codec->codec_id == AV_CODEC_ID_DVD_SUBTITLE) mov_create_dvd_sub_decoder_specific_info(track, st); -else if (!TAG_IS_AVCI(track->tag)){ +else if (!TAG_IS_AVCI(track->tag) && st->codec->codec_id != AV_CODEC_ID_DNXHD) { track->vos_len = st->codec->extradata_size; track->vos_data = av_malloc(track->vos_len); if (!track->vos_data) { -- 1.7.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat: Outputting DNxHD into .mov containers 'corrupts' following atoms until end of stsd
On Fri, Feb 20, 2015 at 11:36 AM, Michael Niedermayer wrote: > applied the case for DNxHD, for the more general case, please > explain which case(s) and software need them, and how to reproduce > that My experience and by the looks of things other people using libquicktime have seen issues with Final Cut having problems reading the files if the stds http://libquicktime.cvs.sourceforge.net/viewvc/libquicktime/libquicktime/src/stsdtable.c?view=markup quicktime_write_stsd_video() line 643 is where they sometimes pad. http://comments.gmane.org/gmane.comp.video.libquicktime.devel/1348 appears to be the discussion around why they do it > I dont see where the text would allow one to add such padding by > ones own choice. I just see a note that says that parsers need to > cope with it. Thats not the same as saying its ok to add it in all > cases. > But maybe iam missing something, i didnt read the whole document no I would agree it only indicates it is optional Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] Gaining access to a encoder context in avformat?
On Fri, Feb 20, 2015 at 2:14 PM, Michael Niedermayer wrote: > On Fri, Feb 20, 2015 at 01:48:55PM +0000, Kevin Wheatley wrote: >> Here is the kind of thing that this looks like... This is definitely >> NOT a patch :-) > > i dont understand this patch there are at least two of us. > please explain why you use vos_data > it can be the first packet and you seem to implement that case > but it is not always the first packet and you seem to add alot of > code to handle this. But if the data is in the dnxhd packets > using that always and not vos_data seems much easier > what am i missing ? probably nothing, I've only tried to find a small change to the code, and to localise the effect to the function so as to minimise the effect of the change - I really don't have a full enough grasp of the code base to do otherwise. > or one could ask the other way around why vos data is set > inconsistently, is there a case where setting it to the first packet > does not work? I'm simply showing how the current contents of vos_data can be used, like you say it would be nice if it was always one or the other, but for whatever reason the copy case puts the atoms in there, which is why the current code without the patch generates incorrect atoms when copy is used on DNxHD files as the code that writes the new atom expects the bitstream, all I did was try a quick detection of what the contents are and use them appropriately. to see if the files would then be fixed. In my case I'm unable to read the files in The Foundry's Nuke if I 'copy' them via ffmpeg because the atom was written incorrectly, but with the diff applied then the copied files will load. Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] Why is writing the colr atom not the default in the mov muxer?
On Fri, Feb 20, 2015 at 1:30 PM, Robert Krüger wrote: > if I read the code correctly, the colr atom is only written in the mov > muxer if the flag write_colr is specified. Was that behaviour chosen to > have better backward compatibility or is there another reason not to write > this standard atom by default? I chose that way to preserve the older behaviour, as it can change how files will be interpreted. Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] Gaining access to a encoder context in avformat?
Here is the kind of thing that this looks like... This is definitely NOT a patch :-) On Fri, Feb 20, 2015 at 1:22 PM, Michael Niedermayer wrote: > On Fri, Feb 20, 2015 at 12:35:59PM +0000, Kevin Wheatley wrote: >> Please excuse my newbie knowledge of the code base BTW... >> >> I've just done a simple test >> >> ffmpeg -color_range mpeg -i test_charts/test_encoding.tif -c dnxhd -vb >> 115M /quicktimes/ffmpeg_test.mov >> >> During this the vos_data contains... >> >> 00 00 02 80 01 01 80 A0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >> 00 04 38 07 80 00 04 38 00 00 38 88 ... >> >> Which looks to me like the start of the VC3/DNxHD bit stream and in >> this case the code puts valid data in the header atoms. >> >> If I then >> >> fmpeg -i quicktimes/ffmpeg_test.mov -c copy quicktimes/ffmpeg_test_copy.mov >> >> the vos_data instead contains >> >> 00 00 00 18 41 43 4C 52 41 43 4C 52 30 30 30 31 00 00 00 01 00 00 00 >> 00 00 00 00 18 41 50 52 47 41 50 52 47 ... >> >> which is the start of the Avid atoms from the incomming Quicktime. >> >> So I could peak into the stream and 'guess' based on the content being >> 0x00, 0x00, 0x02, 0x80, 0x01 that we are encoding and can trust the >> contents, else I could search through looking for the atom via the >> string "ARESARES" and then having located it I could assume to use >> that. The only oddball case is if it is not found, at that point the >> code needs to know if it is interlaced as well as the avid codec >> identifier, or maybe that is the point at which we 'give up' and fail >> with unsupported? > > if you need to put 2 bits from a fixed location of the "first" packet > into some avid atoms its probably easiest to read them straight out of > the first packet into some new field in a struct and then use that > when writing the atoms. > > IIUC theres no generic field in AVStream, AVCodecContext or elsewhere > that holds this information. If there is, setting this field from the > encoder/demuxer and using it would be the more correct path > > extracting the data from vos_data seems trickier > > It would also be possible to add a first_packet field where vos_data > is and always initialize it to the first packet and use that instead > > I sure do not fully know all the details of the issue so i might be > missing something > > [...] > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > There will always be a question for which you do not know the correct answer. > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > diff --git a/libavformat/movenc.c b/libavformat/movenc.c index a72f84e..dc5a7b4 100644 --- a/libavformat/movenc.c +++ b/libavformat/movenc.c @@ -1033,6 +1033,10 @@ static int mov_write_hvcc_tag(AVIOContext *pb, MOVTrack *track) static int mov_write_avid_tag(AVIOContext *pb, MOVTrack *track) { int i; +int interlaced = 0; +int cid = 0; +uint32_t temp; + avio_wb32(pb, 24); /* size */ ffio_wfourcc(pb, "ACLR"); ffio_wfourcc(pb, "ACLR"); @@ -1056,10 +1060,43 @@ static int mov_write_avid_tag(AVIOContext *pb, MOVTrack *track) ffio_wfourcc(pb, "ARES"); ffio_wfourcc(pb, "ARES"); ffio_wfourcc(pb, "0001"); -avio_wb32(pb, AV_RB32(track->vos_data + 0x28)); /* dnxhd cid, some id ? */ + +if (track->vos_data && track->vos_len > 0x29) { +if (track->vos_data[0] == 0x00 && +track->vos_data[1] == 0x00 && +track->vos_data[2] == 0x02 && +track->vos_data[3] == 0x80 && +(track->vos_data[4] == 0x01 ||track->vos_data[4] == 0x02)) { +/* looks like a DNxHD bit stream */ +interlaced = (track->vos_data[5] & 2); +cid = AV_RB32(track->vos_data + 0x28); +} else if (track->vos_len > 120) { /* big enough to contain the atom */ +for (i = 0; i < (track->vos_len - 4); i += 4) { +temp = AV_RL32(track->vos_data + i); +if ((AV_RL32(track->vos_data + i) == MKTAG('A', 'R', 'E', 'S')) && +(AV_RL32(track->vos_data + i + 4) == MKTAG('A', 'R', 'E', 'S'))) +break; +} +if (i < (track->vos_len - 12)) +{ +interlaced = track->enc->field_order > AV_FIELD_PROGRESSIVE; +
Re: [FFmpeg-devel] Gaining access to a encoder context in avformat?
Please excuse my newbie knowledge of the code base BTW... I've just done a simple test ffmpeg -color_range mpeg -i test_charts/test_encoding.tif -c dnxhd -vb 115M /quicktimes/ffmpeg_test.mov During this the vos_data contains... 00 00 02 80 01 01 80 A0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 04 38 07 80 00 04 38 00 00 38 88 ... Which looks to me like the start of the VC3/DNxHD bit stream and in this case the code puts valid data in the header atoms. If I then fmpeg -i quicktimes/ffmpeg_test.mov -c copy quicktimes/ffmpeg_test_copy.mov the vos_data instead contains 00 00 00 18 41 43 4C 52 41 43 4C 52 30 30 30 31 00 00 00 01 00 00 00 00 00 00 00 18 41 50 52 47 41 50 52 47 ... which is the start of the Avid atoms from the incomming Quicktime. So I could peak into the stream and 'guess' based on the content being 0x00, 0x00, 0x02, 0x80, 0x01 that we are encoding and can trust the contents, else I could search through looking for the atom via the string "ARESARES" and then having located it I could assume to use that. The only oddball case is if it is not found, at that point the code needs to know if it is interlaced as well as the avid codec identifier, or maybe that is the point at which we 'give up' and fail with unsupported? Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] Gaining access to a encoder context in avformat?
On Fri, Feb 20, 2015 at 11:44 AM, Michael Niedermayer wrote: > If you start with a existing movie and copy the packets one by one > there is no decoder and no encoder, so no dnxencoder structure > > if you want to put some value in the avid atom which are stored in the > bitstream/packets of dnxhd and not in the generic data structures > like width/height would be then you have to extract these from the > dnxhd bitstream one way or another because thats the only thing thats > there in the memory of your machine. I dont know what exact information > you want/need from the dnxhd encoder ... hmm, maybe I should just say that copy is not supported for DNxHD files then as that sounds a pain to deal with, or I could put better check in the arbitrary buffer offsets the mov_write_avid_tag function uses to access track->vos_data, which is what sent me down the direction of trying to access the encoder data. Is there a sensible function I can use to find a specific atom within the buffer? On a related note when doing a -c copy is it supposed to preserve color range (trc, space, primaries) in the output if it is set on the input/command line? Thanks Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] Gaining access to a encoder context in avformat?
Sent on the go... > On 19 Feb 2015, at 18:35, Michael Niedermayer wrote: > > theres no encoder, the data could instead come from another mov > file or a binary encoder used by some user application with > libavformat Michael, Thanks for your response, I must be confused then. What I thought was based on the following. I have say a set of image files that I want to encode as dnxhd in QuickTime, so ffmpeg sets up the encoder, and during each image's encoding the encoder needs some information such as if the movie is encoded with interlaced fields, and also the specifics of the resolution and bit rates there's a codec/compression Id associated with those settings. When writing the avid atoms, they also should have those same values encoded in them. This is important as if instead of starting from image files we start from an existing movie with the atom present, we would not want to use values based on that atom, which is what s currently done, I wanted to find the dnxencoder structure to correctly populate the atom. Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] Gaining access to a encoder context in avformat?
Hi, I realise this potentially breaks through lots of API boundaries/good taste, but I've noticed a case where the movenc.c code should really ask the specific encoder context (DNXHDEncContext) for some of it's fields to ensure that the atoms written have a consistent setting with what the codec did. Is this something 'allowed'? How best should I access it? thanks Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat: Add simple ACLR atom reading to set the color range
Thanks, just a reminder that reading ffmpeg generated DNxHD in .mov won't work without a variant of my other patch, which correctly places the padding in the stsd atom, I'll pull the latest master and rebase to make sure it still holds. Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat: Add simple ACLR atom reading to set the color range
V3 of the patch, I've attempted to include the general comments from the thread. New to this version, I've reworked the function that reads the atom into the extradata into one which calls 2 helper functions (one to realloc, one to read), I've then reused these functions to read the ACLR atom reliably. My ACLR reading function now calls these functions directly rather than via the mov_read_avid() function, this means that the atom will be used to set the range no matter which codec is used for the track (in the current code the mov_read_avid() function limits it to a restricted set pf codecs). I have not changed how the ACLR is used in the writer as it will still be found in the extradata buffer, this does mean that there maybe cases of 'none avid' codecs that now have the atom and when ouput could have modified behavior but I've never seen one of these files to test with. Thanks for any comments, Kevin From fe6216aec8592baaf40edaa61a73321161548009 Mon Sep 17 00:00:00 2001 From: Kevin Wheatley Date: Thu, 19 Feb 2015 11:08:14 + Subject: [PATCH] Add simple ACLR atom reading to set the color range of the incomming track for codec's like DNxHD that utilise AVID's proprietary atom. On input ACLR will be used to set colour range no matter which codec it is associated with. No change for when it will be output. Rework mov_read_extradata function to allow detection of truncated atom reads by callers. Signed-off-by: Kevin Wheatley --- libavformat/mov.c | 108 +--- 1 files changed, 85 insertions(+), 23 deletions(-) diff --git a/libavformat/mov.c b/libavformat/mov.c index 6d2262a..7718d8e 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -1079,42 +1079,63 @@ static int mov_read_fiel(MOVContext *c, AVIOContext *pb, MOVAtom atom) return 0; } +static int mov_realloc_extradata(AVCodecContext *codec, MOVAtom atom) +{ +int err = 0; +uint64_t size = (uint64_t)codec->extradata_size + atom.size + 8 + FF_INPUT_BUFFER_PADDING_SIZE; +if (size > INT_MAX || (uint64_t)atom.size > INT_MAX) +return AVERROR_INVALIDDATA; +if ((err = av_reallocp(&codec->extradata, size)) < 0) { +codec->extradata_size = 0; +return err; +} +codec->extradata_size = size - FF_INPUT_BUFFER_PADDING_SIZE; +return 0; +} + +/* Read a whole atom into the extradata return the size of the atom read, possibly truncated if != atom.size */ +static int64_t mov_read_atom_into_extradata(MOVContext *c, AVIOContext *pb, MOVAtom atom, +AVCodecContext *codec, uint8_t *buf) +{ +int64_t result = atom.size; +int err; + +AV_WB32(buf, atom.size + 8); +AV_WL32(buf + 4, atom.type); +err = avio_read(pb, buf + 8, atom.size); +if (err < 0) { +return err; +} else if (err < atom.size) { +av_log(c->fc, AV_LOG_WARNING, "truncated extradata\n"); +codec->extradata_size -= atom.size - err; +result = err; +} +memset(buf + 8 + err, 0, FF_INPUT_BUFFER_PADDING_SIZE); +return result; +} + /* FIXME modify qdm2/svq3/h264 decoders to take full atom as extradata */ static int mov_read_extradata(MOVContext *c, AVIOContext *pb, MOVAtom atom, enum AVCodecID codec_id) { AVStream *st; -uint64_t size; -uint8_t *buf; +uint64_t original_size; int err; if (c->fc->nb_streams < 1) // will happen with jp2 files return 0; -st= c->fc->streams[c->fc->nb_streams-1]; +st = c->fc->streams[c->fc->nb_streams-1]; if (st->codec->codec_id != codec_id) return 0; /* unexpected codec_id - don't mess with extradata */ -size= (uint64_t)st->codec->extradata_size + atom.size + 8 + FF_INPUT_BUFFER_PADDING_SIZE; -if (size > INT_MAX || (uint64_t)atom.size > INT_MAX) -return AVERROR_INVALIDDATA; -if ((err = av_reallocp(&st->codec->extradata, size)) < 0) { -st->codec->extradata_size = 0; +original_size = st->codec->extradata_size; +err = mov_realloc_extradata(st->codec, atom); +if (err) return err; -} -buf = st->codec->extradata + st->codec->extradata_size; -st->codec->extradata_size= size - FF_INPUT_BUFFER_PADDING_SIZE; -AV_WB32( buf, atom.size + 8); -AV_WL32( buf + 4, atom.type); -err = avio_read(pb, buf + 8, atom.size); -if (err < 0) { -return err; -} else if (err < atom.size) { -av_log(c->fc, AV_LOG_WARNING, "truncated extradata\n"); -st->codec->extradata_size -= atom.size - err; -} -memset(buf + 8 + err, 0, FF_INPUT_BUFFER_PADDING_SIZE); -return 0; + +err = mov_read_atom_into_extradata(c, pb, atom, st->codec, st->codec->extrada
Re: [FFmpeg-devel] [PATCH] avformat: Add simple ACLR atom reading to set the color range
So reading through the comments in this thread, it looks to me like some of the problems come from the use of the mov_read_extradata() function, which has more logic than the name implies, in particular it will successfully return even if it did not read the atom into the extradata, so if i just directly read the atom, what will break? I could do the ugly thing of reading it and then filling the extradata (or modify the reading function to better communicate to the callers if the data was read correctly). The atom could also not be in the extradata but does it have to be? The movenc.c writer of the atom doesn't always use the extradata - for DNxHD relies on the colour_range of the track's encoder directly and doesn't appear to write the extradata (calls mov_write_avid_tag()), for other codecs it uses mov_write_extradata_tag() (for AV_CODEC_ID_AVU and AV_CODEC_ID_SVQ3). This suggests that for DNxHD I don't need it in the extradata to preserve behaviour, or am I missing something in the code (something in the use as a library for instance)? Thanks Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat: Add simple ACLR atom reading to set the color range
On Tue, Feb 17, 2015 at 12:25 PM, Michael Niedermayer wrote: > if the codec id doesnt match the expected, mov_read_extradata will > return 0 even without any truncation. > In this case the error message would be incorrect So should I code the test against codec id against the files I have or the ones that might be valid, but I don't have samples for? It looks like AV_CODEC_ID_AVUI and AV_CODEC_ID_DNXHD are tested for in other parts of the code, so I'd be comfortable with those. However I'm thinking the call to mov_read_avid might be skipped/inlined if I only tested against the DNxHD codec, there is something that begins to look like a code smell if I duplicate the list of codecs, but I'm happy to go with whatever is preferred. Thanks Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat: Add simple ACLR atom reading to set the color range
Oops, I missed that when I allowed the atom to be added to the end of the extradata, updated. I'm not sure if logging as an error is appropriate in the case when the extradata is not large enough, for this to occur the file will have been truncated somehow and the mov_read_extradata function will have logged a warning and returned 0. Which at first glance seams wrong - should mov_read_extradata return something other than 0 in this case? Either way it does mean the code could read the wrong value, but should not crash with the guard in place. Thanks Kevin From c48f8f2d74562e0e3ee9e6d496fc6a1c4320db14 Mon Sep 17 00:00:00 2001 From: Kevin Wheatley Date: Tue, 17 Feb 2015 11:47:47 + Subject: [PATCH] Add simple ACLR atom reading to set the color range of the incomming track for codec's like DNxHD that utilise AVID's proprietary atom. Added paranoia check for memory reallocation weirdness that might occur (should never trigger but be defensive) Signed-off-by: Kevin Wheatley --- libavformat/mov.c | 36 +++- 1 files changed, 35 insertions(+), 1 deletions(-) diff --git a/libavformat/mov.c b/libavformat/mov.c index 6d2262a..28a140a 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -1178,6 +1178,40 @@ static int mov_read_ares(MOVContext *c, AVIOContext *pb, MOVAtom atom) return mov_read_avid(c, pb, atom); } +static int mov_read_aclr(MOVContext *c, AVIOContext *pb, MOVAtom atom) +{ +int ret = mov_read_avid(c, pb, atom); + +if (!ret && c->fc->nb_streams >= 1) { +AVCodecContext *codec = c->fc->streams[c->fc->nb_streams-1]->codec; +if (atom.size == 16) { +if (codec->extradata_size >= atom.size + 8) { +/* This assumes the atom will be at the end of the extradata */ +const uint8_t range_value = codec->extradata[codec->extradata_size - 5]; +switch (range_value) { +case 1: +codec->color_range = AVCOL_RANGE_MPEG; +break; +case 2: +codec->color_range = AVCOL_RANGE_JPEG; +break; +default: +av_log(c, AV_LOG_WARNING, "unknown aclr value (%d)\n", range_value); +break; +} +av_dlog(c, "color_range: %"PRIu8"\n", codec->color_range); +} else { + /* For some reason the whole atom was not added to the extradata */ + av_log(c, AV_LOG_ERROR, "aclr not decoded - incomplete extradata size %d\n", codec->extradata_size); +} +} else { +av_log(c, AV_LOG_WARNING, "aclr not decoded - unexpected size %ld\n", atom.size); +} +} + +return ret; +} + static int mov_read_svq3(MOVContext *c, AVIOContext *pb, MOVAtom atom) { return mov_read_extradata(c, pb, atom, AV_CODEC_ID_SVQ3); @@ -3390,7 +3424,7 @@ static int mov_read_free(MOVContext *c, AVIOContext *pb, MOVAtom atom) } static const MOVParseTableEntry mov_default_parse_table[] = { -{ MKTAG('A','C','L','R'), mov_read_avid }, +{ MKTAG('A','C','L','R'), mov_read_aclr }, { MKTAG('A','P','R','G'), mov_read_avid }, { MKTAG('A','A','L','P'), mov_read_avid }, { MKTAG('A','R','E','S'), mov_read_ares }, -- 1.7.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avformat: Add simple ACLR atom reading to set the color range
Add simple ACLR atom reading to set the color range of the incomming track for codec's like DNxHD that utilise AVID's proprietary atom. Note: for this to work with ffmpeg generated DNxHD QuickTime files you need to also use my other patch to prevent ffmpeg generating 'corrupt' files. From 561db6b347bed1f60131c3eb2bebe890a402ad63 Mon Sep 17 00:00:00 2001 From: Kevin Wheatley Date: Tue, 17 Feb 2015 09:15:06 + Subject: [PATCH] Add simple ACLR atom reading to set the color range of the incomming track for codec's like DNxHD that utilise AVID's proprietary atom. Signed-off-by: Kevin Wheatley --- libavformat/mov.c | 32 +++- 1 files changed, 31 insertions(+), 1 deletions(-) diff --git a/libavformat/mov.c b/libavformat/mov.c index 6d2262a..0d4b0cf 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -1178,6 +1178,36 @@ static int mov_read_ares(MOVContext *c, AVIOContext *pb, MOVAtom atom) return mov_read_avid(c, pb, atom); } +static int mov_read_aclr(MOVContext *c, AVIOContext *pb, MOVAtom atom) +{ +int ret = mov_read_avid(c, pb, atom); + +if (!ret && c->fc->nb_streams >= 1) { +if (atom.size == 16) { +AVCodecContext *codec = c->fc->streams[c->fc->nb_streams-1]->codec; + +/* This assumes the atom will be at the end of the extradata */ +const uint8_t range_value = codec->extradata[codec->extradata_size - 5]; +switch (range_value) { +case 1: +codec->color_range = AVCOL_RANGE_MPEG; +break; +case 2: +codec->color_range = AVCOL_RANGE_JPEG; +break; +default: +av_log(c, AV_LOG_WARNING, "unknown aclr value (%d)\n", range_value); +break; +} +av_dlog(c, "color_range: %"PRIu8"\n", codec->color_range); +} else { +av_log(c, AV_LOG_WARNING, "aclr not decoded - unexpected size %ld\n", atom.size); +} +} + +return ret; +} + static int mov_read_svq3(MOVContext *c, AVIOContext *pb, MOVAtom atom) { return mov_read_extradata(c, pb, atom, AV_CODEC_ID_SVQ3); @@ -3390,7 +3420,7 @@ static int mov_read_free(MOVContext *c, AVIOContext *pb, MOVAtom atom) } static const MOVParseTableEntry mov_default_parse_table[] = { -{ MKTAG('A','C','L','R'), mov_read_avid }, +{ MKTAG('A','C','L','R'), mov_read_aclr }, { MKTAG('A','P','R','G'), mov_read_avid }, { MKTAG('A','A','L','P'), mov_read_avid }, { MKTAG('A','R','E','S'), mov_read_ares }, -- 1.7.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] compile failure with -DDEBUG of HEAD
This time with patch... On Mon, Feb 16, 2015 at 4:58 PM, Kevin Wheatley wrote: > Whilst compiling with -DDEBUG I get the following... > > libavformat/rtpdec_h264.c: In function 'h264_handle_packet_stap_a': > libavformat/rtpdec_h264.c:208: error: 'data' undeclared (first use in > this function) > libavformat/rtpdec_h264.c:208: error: (Each undeclared identifier is > reported only once > libavformat/rtpdec_h264.c:208: error: for each function it appears in.) > libavformat/rtpdec_h264.c: In function 'h264_handle_packet_fu_a': > libavformat/rtpdec_h264.c:259: error: 'data' undeclared (first use in > this function) > > Looks like passing in the needed context to from the calling functions > would work, > > Kevin diff --git a/libavformat/rtpdec_h264.c b/libavformat/rtpdec_h264.c index 89053ef..24b701c 100644 --- a/libavformat/rtpdec_h264.c +++ b/libavformat/rtpdec_h264.c @@ -177,7 +177,7 @@ static int sdp_parse_fmtp_config_h264(AVFormatContext *s, return 0; } -static int h264_handle_packet_stap_a(AVFormatContext *ctx, AVPacket *pkt, +static int h264_handle_packet_stap_a(AVFormatContext *ctx, PayloadContext *data, AVPacket *pkt, const uint8_t *buf, int len) { int pass = 0; @@ -234,7 +234,7 @@ static int h264_handle_packet_stap_a(AVFormatContext *ctx, AVPacket *pkt, return 0; } -static int h264_handle_packet_fu_a(AVFormatContext *ctx, AVPacket *pkt, +static int h264_handle_packet_fu_a(AVFormatContext *ctx, PayloadContext *data, AVPacket *pkt, const uint8_t *buf, int len) { uint8_t fu_indicator, fu_header, start_bit, nal_type, nal; @@ -308,7 +308,7 @@ static int h264_handle_packet(AVFormatContext *ctx, PayloadContext *data, buf++; len--; // first we are going to figure out the total size -result = h264_handle_packet_stap_a(ctx, pkt, buf, len); +result = h264_handle_packet_stap_a(ctx, data, pkt, buf, len); break; case 25: // STAP-B @@ -322,7 +322,7 @@ static int h264_handle_packet(AVFormatContext *ctx, PayloadContext *data, break; case 28: // FU-A (fragmented nal) -result = h264_handle_packet_fu_a(ctx, pkt, buf, len); +result = h264_handle_packet_fu_a(ctx, data, pkt, buf, len); break; case 30: // undefined ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] compile failure with -DDEBUG of HEAD
Whilst compiling with -DDEBUG I get the following... libavformat/rtpdec_h264.c: In function 'h264_handle_packet_stap_a': libavformat/rtpdec_h264.c:208: error: 'data' undeclared (first use in this function) libavformat/rtpdec_h264.c:208: error: (Each undeclared identifier is reported only once libavformat/rtpdec_h264.c:208: error: for each function it appears in.) libavformat/rtpdec_h264.c: In function 'h264_handle_packet_fu_a': libavformat/rtpdec_h264.c:259: error: 'data' undeclared (first use in this function) Looks like passing in the needed context to from the calling functions would work, Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avformat: Outputting DNxHD into .mov containers 'corrupts' following atoms until end of stsd
ffmpeg and qtdump could not decode pasp/colr atoms in the files made by ffmpeg, when outputting DNxHD due to the incorrect padding placement. Now we add the padding in the correct place, we also always add padding for Final Cut compatibility. Tidy up FATE changes due to padding changes. Kevin From b67ca5347a26227621054c82a688cc0ca44c11a1 Mon Sep 17 00:00:00 2001 From: Kevin Wheatley Date: Mon, 16 Feb 2015 10:40:36 + Subject: [PATCH] Outputting DNxHD into .mov containers 'corrupts' following atoms until end of stsd ffmpeg and qtdump could not decode pasp/colr atoms in the files made by ffmpeg, when outputting DNxHD due to the incorrect padding placement. Now we add the padding in the correct place, we also always add padding for Final Cut compatibility. Tidy up FATE changes due to padding changes. Signed-off-by: Kevin Wheatley --- Changelog |3 ++ libavformat/movenc.c |7 +++- tests/ref/lavf/ismv | 12 +++--- tests/ref/lavf/mov| 16 tests/ref/seek/lavf-mov | 44 tests/ref/vsynth/vsynth1-avui |4 +- tests/ref/vsynth/vsynth1-dnxhd-1080i-colr |2 +- tests/ref/vsynth/vsynth1-mpeg4|4 +- tests/ref/vsynth/vsynth1-prores |4 +- tests/ref/vsynth/vsynth1-prores_ks|4 +- tests/ref/vsynth/vsynth1-qtrle|4 +- tests/ref/vsynth/vsynth1-qtrlegray|4 +- tests/ref/vsynth/vsynth1-svq1 |4 +- tests/ref/vsynth/vsynth2-avui |4 +- tests/ref/vsynth/vsynth2-dnxhd-1080i-colr |2 +- tests/ref/vsynth/vsynth2-mpeg4|4 +- tests/ref/vsynth/vsynth2-prores |4 +- tests/ref/vsynth/vsynth2-prores_ks|4 +- tests/ref/vsynth/vsynth2-qtrle|4 +- tests/ref/vsynth/vsynth2-qtrlegray|4 +- tests/ref/vsynth/vsynth2-svq1 |4 +- tests/ref/vsynth/vsynth3-dnxhd-1080i-colr |2 +- tests/ref/vsynth/vsynth3-mpeg4|4 +- tests/ref/vsynth/vsynth3-prores |4 +- tests/ref/vsynth/vsynth3-prores_ks|4 +- tests/ref/vsynth/vsynth3-qtrle|4 +- tests/ref/vsynth/vsynth3-svq1 |4 +- tests/ref/vsynth/vsynth_lena-avui |4 +- tests/ref/vsynth/vsynth_lena-dnxhd-1080i-colr |2 +- 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 +- 35 files changed, 100 insertions(+), 94 deletions(-) diff --git a/Changelog b/Changelog index 0bf7018..8080286 100644 --- a/Changelog +++ b/Changelog @@ -30,6 +30,9 @@ version : - DV RTP payload format (RFC 6469) depacketizer - DXVA2-accelerated HEVC decoding - AAC ELD 480 decoding +- Fix stsd atom corruption in DNxHD QuickTimes +- Add 4 bytes stsd zero padding to video track for all QuickTime outputs + version 2.5: - HEVC/H.265 RTP payload format (draft v6) packetizer diff --git a/libavformat/movenc.c b/libavformat/movenc.c index f95d066..e8b3a6b 100644 --- a/libavformat/movenc.c +++ b/libavformat/movenc.c @@ -1077,8 +1077,6 @@ static int mov_write_avid_tag(AVIOContext *pb, MOVTrack *track) for (i = 0; i < 10; i++) avio_wb64(pb, 0); -/* extra padding for stsd needed */ -avio_wb32(pb, 0); return 0; } @@ -1674,6 +1672,11 @@ static int mov_write_video_tag(AVIOContext *pb, MOVMuxContext *mov, MOVTrack *tr mov_write_pasp_tag(pb, track); } +/* extra padding for stsd needed */ +/* https://developer.apple.com/library/mac/documentation/QuickTime/QTFF/QTFFChap2/qtff2.html#//apple_ref/doc/uid/TP4939-CH204-61112 */ +/* suggests it is optional, but there are suggestions that Final Cut may need it. */ +avio_wb32(pb, 0); + return update_size(pb, pos); } diff --git a/tests/ref/lavf/ismv b/tests/ref/lavf/ismv index f29b5ff..ea8ea3a 100644 --- a/tests/ref/lavf/ismv +++ b/tests/ref/lavf/ismv @@ -1,9 +1,9 @@ -a9ccbb4cd1436d222ef4425567b4e03d *./tests/data/lavf/lavf.ismv -312542 ./tests/data/lavf/lavf.ismv +e26cf1bc88f31010c4c34af8eb9580ec *./tests/data/lavf/lavf.ismv +312546 ./tests/data/lavf/lavf.ismv ./tests/data/lavf/lavf.ismv CRC=0x9d9a638a -440d85f9fd5b9f63c2676638782b5c15 *./tests/data/lavf/lavf.ismv -321448 ./tests/data/lavf/lavf.ismv +ed0124f2dab1204869f3afed20d631c9 *./tests/data/lavf/lavf.ismv +321452 ./tests/data/lavf/lavf.ismv ./tests/data/lavf/lavf.ismv CRC=0xe8130120 -a9ccbb4cd1436d222ef4425567b4e03d *./tests/data/lavf/lavf.ismv -312542 ./tests/data/lavf/lavf.ismv +e26cf1bc88f31010c4c
Re: [FFmpeg-devel] avformat: QuickTime Sample Description Table 'corruption' with ACLR atom.
I've rebased the patch against current master, it is a lot of FATE changes due to the extra 4 bytes... It should now merge appropriately, see https://github.com/FFmpeg/FFmpeg/pull/110 Please let me know if anything needs tweaking Thanks Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] avformat: QuickTime Sample Description Table 'corruption' with ACLR atom.
OK I'll proceed with that. Thanks Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] avformat: QuickTime Sample Description Table 'corruption' with ACLR atom.
On Fri, Feb 13, 2015 at 3:06 PM, Michael Niedermayer wrote: > On Fri, Feb 13, 2015 at 02:43:13PM +0000, Kevin Wheatley wrote: > what difference does the terminator make ? > does it improve compatibility with other software ? I believe there are some versions of Apple's Final Cut that need the terminator, I've also been wondering if mp4 has the same needs but I don't have the spec to cross check. Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] avformat: QuickTime Sample Description Table 'corruption' with ACLR atom.
Hi, currently when writing ACLR atoms to .mov's there is a 'corruption' caused by the function mov_write_avid_tag() writing an additional 4 bytes of zero's. this is potentially then followed by other atoms colr and pasp. Looking at the specifications it appears this 4 bytes is supposed to occur at the end of the stsd if at all, as it is optional. I'm working on a patch to fix the 'corruption' but wondered how to handle the optional terminator: 1) Don't write one ever 2) Write one if requested by adding a flag to -movflags stdsterminator 3) Write one by default, add a 'nostdsterminator' flag (needs a better name) 4) always write one. I'm favouring 2 or 3 Currently I've done option 1 see https://github.com/KevinJW/FFmpeg/compare/mov_dnxhd_stsd_corruption Thanks Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] libavformat: DNxHD in .mov, switch unspecified color_range to mpeg
From 533523210ae02b80d4450793fd9e3865e716e858 Mon Sep 17 00:00:00 2001 From: Kevin Wheatley Date: Tue, 10 Feb 2015 11:37:00 + Subject: [PATCH] libavformat: DNxHD in .mov, switch unspecified color_range to mpeg Avid prefers mpeg range [16-235] by default this change brings ffmpeg into line with that. To obtain the old behaviour use '-color_range jpeg' on the command line prior to the ouput filename. Signed-off-by: Kevin Wheatley --- Changelog |1 + libavformat/movenc.c |3 ++- tests/ref/vsynth/vsynth1-dnxhd-1080i |2 +- tests/ref/vsynth/vsynth1-dnxhd-1080i-colr |2 +- tests/ref/vsynth/vsynth2-dnxhd-1080i |2 +- tests/ref/vsynth/vsynth2-dnxhd-1080i-colr |2 +- tests/ref/vsynth/vsynth3-dnxhd-1080i-colr |2 +- tests/ref/vsynth/vsynth_lena-dnxhd-1080i |2 +- tests/ref/vsynth/vsynth_lena-dnxhd-1080i-colr |2 +- 9 files changed, 10 insertions(+), 8 deletions(-) diff --git a/Changelog b/Changelog index 8e55969..ca5e089 100644 --- a/Changelog +++ b/Changelog @@ -19,6 +19,7 @@ version : - Twofish symmetric block cipher - Support DNx100 (960x720@8) - removed libmpcodecs +- Changed default DNxHD colour range in QuickTime .mov derivatives to mpeg range version 2.5: diff --git a/libavformat/movenc.c b/libavformat/movenc.c index df70d57..f95d066 100644 --- a/libavformat/movenc.c +++ b/libavformat/movenc.c @@ -1037,7 +1037,8 @@ static int mov_write_avid_tag(AVIOContext *pb, MOVTrack *track) ffio_wfourcc(pb, "ACLR"); ffio_wfourcc(pb, "ACLR"); ffio_wfourcc(pb, "0001"); -if (track->enc->color_range == AVCOL_RANGE_MPEG) { /* Legal range (16-235) */ +if (track->enc->color_range == AVCOL_RANGE_MPEG || /* Legal range (16-235) */ +track->enc->color_range == AVCOL_RANGE_UNSPECIFIED) { avio_wb32(pb, 1); /* Corresponds to 709 in official encoder */ } else { /* Full range (0-255) */ avio_wb32(pb, 2); /* Corresponds to RGB in official encoder */ diff --git a/tests/ref/vsynth/vsynth1-dnxhd-1080i b/tests/ref/vsynth/vsynth1-dnxhd-1080i index f9b7e0e..28d55b6 100644 --- a/tests/ref/vsynth/vsynth1-dnxhd-1080i +++ b/tests/ref/vsynth/vsynth1-dnxhd-1080i @@ -1,4 +1,4 @@ -14f1ea20bbd3024ccbfd84c681888d07 *tests/data/fate/vsynth1-dnxhd-1080i.mov +a0234e0a8516d958f423b119aa9e35c4 *tests/data/fate/vsynth1-dnxhd-1080i.mov 3031911 tests/data/fate/vsynth1-dnxhd-1080i.mov a09132c6db44f415e831dcaa630a351b *tests/data/fate/vsynth1-dnxhd-1080i.out.rawvideo stddev:6.29 PSNR: 32.15 MAXDIFF: 64 bytes: 7603200/ 760320 diff --git a/tests/ref/vsynth/vsynth1-dnxhd-1080i-colr b/tests/ref/vsynth/vsynth1-dnxhd-1080i-colr index 5971f33..ee21e96 100644 --- a/tests/ref/vsynth/vsynth1-dnxhd-1080i-colr +++ b/tests/ref/vsynth/vsynth1-dnxhd-1080i-colr @@ -1,4 +1,4 @@ -b6fbfdfe7027fde6853930abad87eaab *tests/data/fate/vsynth1-dnxhd-1080i-colr.mov +200a881696cc70c36c33fb415ed9488b *tests/data/fate/vsynth1-dnxhd-1080i-colr.mov 3031929 tests/data/fate/vsynth1-dnxhd-1080i-colr.mov 5835dff88cb84e83bbe70b5ed5edd5ab *tests/data/fate/vsynth1-dnxhd-1080i-colr.out.rawvideo stddev:5.79 PSNR: 32.87 MAXDIFF: 56 bytes: 7603200/ 760320 diff --git a/tests/ref/vsynth/vsynth2-dnxhd-1080i b/tests/ref/vsynth/vsynth2-dnxhd-1080i index 527e6a7..c3a5073 100644 --- a/tests/ref/vsynth/vsynth2-dnxhd-1080i +++ b/tests/ref/vsynth/vsynth2-dnxhd-1080i @@ -1,4 +1,4 @@ -d59d5d40ffb33ebb9e9e0d5025aefb88 *tests/data/fate/vsynth2-dnxhd-1080i.mov +2b75889122f8d918e1b068d128b618ca *tests/data/fate/vsynth2-dnxhd-1080i.mov 3031911 tests/data/fate/vsynth2-dnxhd-1080i.mov 099001db73036eeb9545c463cf90f0ba *tests/data/fate/vsynth2-dnxhd-1080i.out.rawvideo stddev:1.53 PSNR: 44.43 MAXDIFF: 31 bytes: 7603200/ 760320 diff --git a/tests/ref/vsynth/vsynth2-dnxhd-1080i-colr b/tests/ref/vsynth/vsynth2-dnxhd-1080i-colr index 0994820..d41b5b3 100644 --- a/tests/ref/vsynth/vsynth2-dnxhd-1080i-colr +++ b/tests/ref/vsynth/vsynth2-dnxhd-1080i-colr @@ -1,4 +1,4 @@ -d510bc0d58c7cae875e3e67023771d6f *tests/data/fate/vsynth2-dnxhd-1080i-colr.mov +53dfdc17882f2240a10c799287bf4b68 *tests/data/fate/vsynth2-dnxhd-1080i-colr.mov 3031929 tests/data/fate/vsynth2-dnxhd-1080i-colr.mov e4cf5528c993b5e7d57a9d0a4d2cd0c6 *tests/data/fate/vsynth2-dnxhd-1080i-colr.out.rawvideo stddev:1.58 PSNR: 44.15 MAXDIFF: 33 bytes: 7603200/ 760320 diff --git a/tests/ref/vsynth/vsynth3-dnxhd-1080i-colr b/tests/ref/vsynth/vsynth3-dnxhd-1080i-colr index 926a7b2..2c5084c 100644 --- a/tests/ref/vsynth/vsynth3-dnxhd-1080i-colr +++ b/tests/ref/vsynth/vsynth3-dnxhd-1080i-colr @@ -1,4 +1,4 @@ -3b06d8675f9623db77b6a42916663608 *tests/data/fate/vsynth3-dnxhd-1080i-colr.mov +da84414ce38ed0479c61a493398c912a *tests/data/fate/vsynth3-dnxhd-1080i-colr.mov 3031929 tests/data/fate/vsynth3-dnxhd-1080i-colr.mov 7dd6b261e439cda21df4f01b45336b41 *tests/data/fate/vsynth3-
Re: [FFmpeg-devel] Default color range encoding in Avid ACLR tags embeedded in QuickTime .movs
On Tue, Feb 10, 2015 at 10:34 AM, Michael Niedermayer wrote: > if theres no way to store unknown range then your suggestion sounds > reasonable, can you send a patch? I'm not aware of a value to specify in the ACLR atom for unspecified - I could guess at a value of 0 but that is pure speculation, Avid directly (or via the official QuickTime codecs) only ever uses the values of 1 or 2 from what I can see. Patch yes I can do that, should this be a simple if statement modification e.g. if (track->enc->color_range == AVCOL_RANGE_MPEG || /* Legal range (16-235) */ track->enc->color_range == AVCOL_RANGE_UNSPECIFIED) { avio_wb32(pb, 1); /* Corresponds to 709 in official encoder */ } else { /* Full range (0-255) */ avio_wb32(pb, 2); /* Corresponds to RGB in official encoder */ } or does ffmpeg favor a switch/multiple if to handle each of the cases? > is there some advantage in doing that default handling specific to > the codec_id ? I'm not sure what you mean, in this case the atom being written is specific to certain codecs (although currently ffmpeg only does this for DNxHD) Thanks Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] Default color range encoding in Avid ACLR tags embeedded in QuickTime .movs
Hi, Just looking through the FATE tests and the changes I wanted to make on reading ACLR atoms and I came across an issue. I wondered what the expected encoding range is for Y'CbCr during the FATE test runs as well as more generally. It would appear that the assumption that the range should be AVCOL_RANGE_MPEG [16-235] during test runs. Unfortunately the DNxHD code assumes the opposite unless you specify (libavformat/movenc.c near line 1038). I would consider this logic to favor full range in the Avid tags incorrect and believe it should be reversed to assume [16-235] unless told full range. FWIW, the default within an Avid is 16-235 for optimal use of DNxHD. Thoughts? Thanks Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] What is the correct way to read Avid ACLR atom?
On Thu, Feb 5, 2015 at 4:24 PM, Carl Eugen Hoyos wrote: > Kevin Wheatley gmail.com> writes: > >> +int ret = mov_read_avid(c, pb, atom); >> // should we do this or read the atom directly >> using avio_*() and not store it in extradata? > > Not storing the atom in extradata would break > remuxing. ah, so the answer to my comment is quite clear now > Did you test your patch? If it works, what > advantage would the decoder patch(es) have? > There would be a few iirc, no? I assume there would need to be put in different files, as an example of why I thought the mov decoder would be a good choice is that I am not sure if DNxHD when wrapped in MXF has a similar choice of encoding ranges - by default the typical Avid work flow is to use '709' encoding range, but sometimes our clients want 'RGB'. As to testing the patch, yes in a few ways, but not exhaustively - mostly verifying ffprobe correctly outputs the encoding range on a variety of files as well as outputting PNGs with the expected range expansion on '709' content, but none on 'RGB' via ffmpeg -i qt_dnxhd.mov blah.%04d.png. I haven't looked at anything more fancy, although I did note that -c copy does not propagate the colour_range but I've not looked at why. Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] What is the correct way to read Avid ACLR atom?
On Thu, Feb 5, 2015 at 3:55 PM, wm4 wrote: > Seeing how the patch actually turned out, I'd say it should be in the > decoder. It's not necessarily mp4 specific, or is it? In this case I'm exporting the QuickTimes from an Avid Media Composer so they are DNxHD in .mov, and in .mp4 with both 'RGB' and '709' settings in Avid speak. From reading the ffmpeg code it looks like other 'Avid' codecs also use similar atoms inside .mov derivatives, but I'm not personally aware if any other container format has them. My question about code style as regards the extradata was me thinking, if I have read the data and done something with it, it isn't really 'extra' anymore but I'm not very familiar with ffmpeg's coding style and architecture. Putting this function in the avformat side of the code allows any codec to pick it up and looked to be the most contained change, but it potentially does change the image processing as now it can determine the encoding range things may get handled differently - this is part of my goal which is to correctly transport the colour metadata across a number of different codecs and containers so that appropriate conversions can be applied. Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] What is the correct way to read Avid ACLR atom?
Hi, so I had a quick go at this but I'm still not sure on the preferred way of reading this, should I read the atom directly and not put the data in extradata? Obviously this doesn't include fixing up the fate data Thanks Kevin On Wed, Feb 4, 2015 at 11:51 AM, Kevin Wheatley wrote: > Hi, > > I've been looking at what the appropriate place and method to read in > the ACLR atom placed in the Avid codecs so that we can automatically > set the color_range. > > From my reading of the code the mov.c code the parse table shunts the > reading of the atom to mov_read_avid(), which in turn calls > mov_read_extradata() which leaves the data in the codec extradata > buffer. > > The issue would be at which point is the preferred point to look into > that buffer and set the colour_range, should it be done in the > specific codec's file in libavcodec (I'm only interested in DNxHD but > I assume the other Avid codecs might need it), or because of this > duplication should the change be made in libavformat/mov.c somewhere > like mov_read_avid() in a similar way to the mov_read_ares() > functions? > > Thanks > > Kevin diff --git a/libavformat/mov.c b/libavformat/mov.c index e5dd1bd..8710742 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -1172,6 +1172,27 @@ static int mov_read_ares(MOVContext *c, AVIOContext *pb, MOVAtom atom) return mov_read_avid(c, pb, atom); } +static int mov_read_aclr(MOVContext *c, AVIOContext *pb, MOVAtom atom) +{ +int ret = mov_read_avid(c, pb, atom); // should we do this or read the atom directly using avio_*() and not store it in extradata? +if (c->fc->nb_streams >= 1) { +AVCodecContext *codec = c->fc->streams[c->fc->nb_streams-1]->codec; +if (codec->extradata_size == 24) +{ +switch (codec->extradata[19]) { +case 1: +codec->color_range = AVCOL_RANGE_MPEG; +break; +case 2: +codec->color_range = AVCOL_RANGE_JPEG; +break; +} +} +} + +return ret; +} + static int mov_read_svq3(MOVContext *c, AVIOContext *pb, MOVAtom atom) { return mov_read_extradata(c, pb, atom, AV_CODEC_ID_SVQ3); @@ -3372,7 +3393,7 @@ static int mov_read_free(MOVContext *c, AVIOContext *pb, MOVAtom atom) } static const MOVParseTableEntry mov_default_parse_table[] = { -{ MKTAG('A','C','L','R'), mov_read_avid }, +{ MKTAG('A','C','L','R'), mov_read_aclr }, { MKTAG('A','P','R','G'), mov_read_avid }, { MKTAG('A','A','L','P'), mov_read_avid }, { MKTAG('A','R','E','S'), mov_read_ares }, ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] What is the correct way to read Avid ACLR atom?
Hi, I've been looking at what the appropriate place and method to read in the ACLR atom placed in the Avid codecs so that we can automatically set the color_range. >From my reading of the code the mov.c code the parse table shunts the reading of the atom to mov_read_avid(), which in turn calls mov_read_extradata() which leaves the data in the codec extradata buffer. The issue would be at which point is the preferred point to look into that buffer and set the colour_range, should it be done in the specific codec's file in libavcodec (I'm only interested in DNxHD but I assume the other Avid codecs might need it), or because of this duplication should the change be made in libavformat/mov.c somewhere like mov_read_avid() in a similar way to the mov_read_ares() functions? Thanks Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] libavformat/movenc.c: Correct color range when writing DNxHD atoms
On Tue, Jan 27, 2015 at 9:27 PM, Michael Niedermayer wrote: > On Tue, Jan 27, 2015 at 11:15:40AM -0800, jon morley wrote: >> movenc.c |9 ++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> 6317011578bca8bf065f5bd4de2dfce803557e81 >> 0001-libavformat-movenc.c-Correct-color-range-when-writin.patch >> From 0097277471810ab1d9d737c64a57c2278a039153 Mon Sep 17 00:00:00 2001 >> From: Jon Morley >> Date: Tue, 27 Jan 2015 11:10:27 -0800 >> Subject: [PATCH] libavformat/movenc.c: Correct color range when writing DNxHD >> atoms >> >> The meaning of the color range values in the AVdn.ACLR atom was swapped. >> This change makes the selection explicit and correctly mapped. >> --- >> libavformat/movenc.c | 9 ++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) > > breaks "make fate" > you need to update the checksums > also please explain in the commit message what referece you used to > know which is the correct value Jon, Michael, this may or may not be the correct setting, I've generated a few different range encodings from different apps and found there is some inconsistency (who'd have thought QuickTime could be ambiguous) in what different apps do, I thought that the values might need to be switched too, however files generated with an Avid appear to suggest ffmpeg's current behavior might be correct (for encoding). I'm going to try get some form of official statement/info direct from Avid, mean while I'm going to go find a few more encoders from third parties and see what they do. Kevin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel