[FFmpeg-devel] libavformat/movenc.c: Correct color range when writing DNxHD atoms
From 0097277471810ab1d9d737c64a57c2278a039153 Mon Sep 17 00:00:00 2001 From: Jon Morley j...@tweaksoftware.com 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(-) diff --git a/libavformat/movenc.c b/libavformat/movenc.c index d7ae5f0..dfe4c27 100644 --- a/libavformat/movenc.c +++ b/libavformat/movenc.c @@ -1039,10 +1039,13 @@ static int mov_write_avid_tag(AVIOContext *pb, MOVTrack *track) ffio_wfourcc(pb, ACLR); ffio_wfourcc(pb, 0001); if (track-enc-color_range == AVCOL_RANGE_MPEG) { /* Legal range (16-235) */ -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 */ +avio_wb32(pb, 2); /* Corresponds to 709 in official encoder */ +} else if (track-enc-color_range == AVCOL_RANGE_JPEG) { /* Full range (0-255) */ +avio_wb32(pb, 1); /* Corresponds to RGB in official encoder */ +} else { +avio_wb32(pb, 0); /* Unspecified */ } + avio_wb32(pb, 0); /* unknown */ avio_wb32(pb, 24); /* size */ -- 1.8.5.2 (Apple Git-48) ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] Question about supported_fps in libavutil/timecode.c::check_fps
Hi Clément, I am sorry I was rude. That was not my intention. I was attempting to follow these directions from the ffmpeg.org page: You can use the FFmpeg libraries in your commercial program, but you are encouraged to publish any patch you make. In this case the best way to proceed is to send your patches to the ffmpeg-devel mailing list following the guidelines illustrated in the remainder of this document. I will stick to mailing patches exclusively in the future. Patches reflecting your suggestions attached. Sincerely, Jon On 1/24/15 8:21 AM, Clément Bœsch wrote: On Sat, Jan 24, 2015 at 07:40:38AM -0800, jon morley wrote: Hi Clément, Hi, That is a good point! I am attaching an additional patch to remove those cases even before entering the mod test loop. Now the logic should look like this: static int check_fps(int fps) { if (fps = 0) return -1; int i; static const int supported_fps_bases[] = {24, 25, 30}; You can't put statements before declarations, some compilers will choke on it. Also, please squash it with the previous patch since it wasn't applied yet. for (i = 0; i FF_ARRAY_ELEMS(supported_fps_bases); i++) if (fps % supported_fps_bases[i] == 0) return 0; return -1; } I am still really curious to know if switching to this division (modulo) test breaks the spirit of check_fps. I could not find anywhere that benefited from the explicit list the method currently used, but that doesn't mean it isn't out there. I'm more concerned about how the rest of the code will behave. Typically, av_timecode_adjust_ntsc_framenum2() could benefit from some improvements (checking if fps % 30, and deducing drop_frames and frames_per_10mins accordingly) if you allow such thing. Then you might need to adjust check_timecode() as well to allow the drop frame for the other % 30. Thanks, Jon [...] Note: please do not top post on this mailing list, it is considered rude. Regards, ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel From 95f1fb3695f086de1baa301015985742d688a159 Mon Sep 17 00:00:00 2001 From: Jon Morley j...@tweaksoftware.com Date: Sat, 24 Jan 2015 12:18:50 -0800 Subject: [PATCH] libavutil/timecode.c: Add support for frame rates beyond 60 fps Instead of looking for specifically supported frame rates this collection of changes checks to see if the given rates are evenly divisible by supported common factors. --- libavutil/timecode.c | 33 - 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/libavutil/timecode.c b/libavutil/timecode.c index 1dfd040..c2469c0 100644 --- a/libavutil/timecode.c +++ b/libavutil/timecode.c @@ -33,18 +33,15 @@ int av_timecode_adjust_ntsc_framenum2(int framenum, int fps) { -/* only works for NTSC 29.97 and 59.94 */ +int factor = 1; int drop_frames = 0; int d, m, frames_per_10mins; -if (fps == 30) { -drop_frames = 2; -frames_per_10mins = 17982; -} else if (fps == 60) { -drop_frames = 4; -frames_per_10mins = 35964; -} else -return framenum; +if (fps 30 || fps % 30 != 0) return framenum; + +factor = fps / 30; +drop_frames = factor * 2; +frames_per_10mins = factor * 17982; d = framenum / frames_per_10mins; m = framenum % frames_per_10mins; @@ -141,10 +138,12 @@ char *av_timecode_make_mpeg_tc_string(char *buf, uint32_t tc25bit) static int check_fps(int fps) { int i; -static const int supported_fps[] = {24, 25, 30, 48, 50, 60}; +static const int supported_fps_multiples[] = {24, 25, 30}; + +if (fps = 0) return -1; -for (i = 0; i FF_ARRAY_ELEMS(supported_fps); i++) -if (fps == supported_fps[i]) +for (i = 0; i FF_ARRAY_ELEMS(supported_fps_multiples); i++) +if (fps % supported_fps_multiples[i] == 0) return 0; return -1; } @@ -155,8 +154,8 @@ static int check_timecode(void *log_ctx, AVTimecode *tc) av_log(log_ctx, AV_LOG_ERROR, Timecode frame rate must be specified\n); return AVERROR(EINVAL); } -if ((tc-flags AV_TIMECODE_FLAG_DROPFRAME) tc-fps != 30 tc-fps != 60) { -av_log(log_ctx, AV_LOG_ERROR, Drop frame is only allowed with 3/1001 or 6/1001 FPS\n); +if ((tc-flags AV_TIMECODE_FLAG_DROPFRAME) tc-fps % 30 != 0) { +av_log(log_ctx, AV_LOG_ERROR, Drop frame is only allowed in frame rates evenly divisible by 30 FPS\n); return AVERROR(EINVAL); } if (check_fps(tc-fps) 0) { @@ -201,9 +200,9 @@ int av_timecode_init_from_string(AVTimecode *tc, AVRational rate, const char *st } memset(tc, 0, sizeof(*tc)); -tc-flags = c != ':' ? AV_TIMECODE_FLAG_DROPFRAME : 0; // drop if ';', '.', ... -tc-rate = rate; -tc-fps = fps_from_frame_rate(rate); +tc-flags = c != ':' ? AV_TIMECODE_FLAG_DROPFRAME : 0
Re: [FFmpeg-devel] Question about supported_fps in libavutil/timecode.c::check_fps
Hi Clément, That is a good point! I am attaching an additional patch to remove those cases even before entering the mod test loop. Now the logic should look like this: static int check_fps(int fps) { if (fps = 0) return -1; int i; static const int supported_fps_bases[] = {24, 25, 30}; for (i = 0; i FF_ARRAY_ELEMS(supported_fps_bases); i++) if (fps % supported_fps_bases[i] == 0) return 0; return -1; } I am still really curious to know if switching to this division (modulo) test breaks the spirit of check_fps. I could not find anywhere that benefited from the explicit list the method currently used, but that doesn't mean it isn't out there. Thanks, Jon On 1/24/15 2:27 AM, Clément Bœsch wrote: On Fri, Jan 23, 2015 at 08:48:37AM -0800, jon morley wrote: Patch attached for consideration. On 1/23/15 8:03 AM, jon morley wrote: Currently check_fps has the following logic: static int check_fps(int fps) { int i; static const int supported_fps[] = {24, 25, 30, 48, 50, 60}; for (i = 0; i FF_ARRAY_ELEMS(supported_fps); i++) if (fps == supported_fps[i]) return 0; return -1; } I am starting to see more and more movies with fps rates in excess of this list from modified GoPro files and other raw camera sources. I was originally adding more entries as the sources came rolling in because I could not see any issue in how this was getting called later with that approach. I still don't see the drawback of adding more, but I am tired of adding a new rate every time I encounter one in the wild. I was curious if it wouldn't make more sense to change the logic to the following: static int check_fps(int fps) { int i; static const int supported_fps_bases[] = {24, 25, 30}; for (i = 0; i FF_ARRAY_ELEMS(supported_fps_bases); i++) if (fps % supported_fps_bases[i] == 0) return 0; return -1; } If that makes sense to you, then I will submit a patch. Please let me know if I have overlooked some other usage/meaning of check_fps that I am overlooking. Thanks, Jon ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel From 73e5339ec76305d34214b5e84dc5a38673f784b7 Mon Sep 17 00:00:00 2001 From: Jon Morley j...@tweaksoftware.com Date: Fri, 23 Jan 2015 08:43:33 -0800 Subject: [PATCH] libavutil/timecode.c: Extend check_fps logic to handle high frame rates QuickTime sources continue to push higher and higher frame rates. This change moves away from explictly testing incoming fps values towards ensuring the incoming value is evenly divisable by 24, 25, or 30. --- libavutil/timecode.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/libavutil/timecode.c b/libavutil/timecode.c index 1dfd040..c10895c 100644 --- a/libavutil/timecode.c +++ b/libavutil/timecode.c @@ -141,10 +141,10 @@ char *av_timecode_make_mpeg_tc_string(char *buf, uint32_t tc25bit) static int check_fps(int fps) { int i; -static const int supported_fps[] = {24, 25, 30, 48, 50, 60}; +static const int supported_fps_bases[] = {24, 25, 30}; -for (i = 0; i FF_ARRAY_ELEMS(supported_fps); i++) -if (fps == supported_fps[i]) +for (i = 0; i FF_ARRAY_ELEMS(supported_fps_bases); i++) +if (fps % supported_fps_bases[i] == 0) return 0; return -1; I don't think you want to accept fps ≤ 0 [...] ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel From 0a72d78992bbeb6c2536285397149cceb64b05d8 Mon Sep 17 00:00:00 2001 From: Jon Morley j...@tweaksoftware.com Date: Sat, 24 Jan 2015 07:28:40 -0800 Subject: [PATCH 2/2] libavutil/timecode.c: check_fps must reject rates at or below zero An earlier change to check_fps's logic which now confirms that the incoming evaluation fps is evenly divisable by a list of supported rates leaves open the possibility of accepting zero and negative frame rates. This change removes that posibility. --- libavutil/timecode.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libavutil/timecode.c b/libavutil/timecode.c index c10895c..446e2d5 100644 --- a/libavutil/timecode.c +++ b/libavutil/timecode.c @@ -140,6 +140,8 @@ char *av_timecode_make_mpeg_tc_string(char *buf, uint32_t tc25bit) static int check_fps(int fps) { +if (fps = 0) return -1; + int i; static const int supported_fps_bases[] = {24, 25, 30}; -- 1.8.5.2 (Apple Git-48) ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] Question about supported_fps in libavutil/timecode.c::check_fps
Patch attached for consideration. On 1/23/15 8:03 AM, jon morley wrote: Currently check_fps has the following logic: static int check_fps(int fps) { int i; static const int supported_fps[] = {24, 25, 30, 48, 50, 60}; for (i = 0; i FF_ARRAY_ELEMS(supported_fps); i++) if (fps == supported_fps[i]) return 0; return -1; } I am starting to see more and more movies with fps rates in excess of this list from modified GoPro files and other raw camera sources. I was originally adding more entries as the sources came rolling in because I could not see any issue in how this was getting called later with that approach. I still don't see the drawback of adding more, but I am tired of adding a new rate every time I encounter one in the wild. I was curious if it wouldn't make more sense to change the logic to the following: static int check_fps(int fps) { int i; static const int supported_fps_bases[] = {24, 25, 30}; for (i = 0; i FF_ARRAY_ELEMS(supported_fps_bases); i++) if (fps % supported_fps_bases[i] == 0) return 0; return -1; } If that makes sense to you, then I will submit a patch. Please let me know if I have overlooked some other usage/meaning of check_fps that I am overlooking. Thanks, Jon ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel From 73e5339ec76305d34214b5e84dc5a38673f784b7 Mon Sep 17 00:00:00 2001 From: Jon Morley j...@tweaksoftware.com Date: Fri, 23 Jan 2015 08:43:33 -0800 Subject: [PATCH] libavutil/timecode.c: Extend check_fps logic to handle high frame rates QuickTime sources continue to push higher and higher frame rates. This change moves away from explictly testing incoming fps values towards ensuring the incoming value is evenly divisable by 24, 25, or 30. --- libavutil/timecode.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/libavutil/timecode.c b/libavutil/timecode.c index 1dfd040..c10895c 100644 --- a/libavutil/timecode.c +++ b/libavutil/timecode.c @@ -141,10 +141,10 @@ char *av_timecode_make_mpeg_tc_string(char *buf, uint32_t tc25bit) static int check_fps(int fps) { int i; -static const int supported_fps[] = {24, 25, 30, 48, 50, 60}; +static const int supported_fps_bases[] = {24, 25, 30}; -for (i = 0; i FF_ARRAY_ELEMS(supported_fps); i++) -if (fps == supported_fps[i]) +for (i = 0; i FF_ARRAY_ELEMS(supported_fps_bases); i++) +if (fps % supported_fps_bases[i] == 0) return 0; return -1; } -- 1.8.5.2 (Apple Git-48) ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel