Re: [FFmpeg-devel] [PATCH] vf_lut: Add support for RGB48 and RGBA64
On Thu, Oct 29, 2015 at 09:41:21PM +0100, Paul B Mahol wrote: > On 10/25/15, Steven Robertson wrote: > > Done, thanks. > > > > probably ok applied thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Asymptotically faster algorithms should always be preferred if you have asymptotical amounts of data signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] ffmpeg.c can not process invalid PTS
diff --git a/ffmpeg.c b/ffmpeg.c
index f91fb7b..22766b3 100644
--- a/ffmpeg.c
+++ b/ffmpeg.c
@@ -3864,6 +3864,9 @@ static int process_input(int file_index)
if (pkt.dts != AV_NOPTS_VALUE)
pkt.dts *= ist->ts_scale;
+if(pkt.pts == AV_NOPTS_VALUE)
+pkt.pts = ist->next_pts;
+
if ((ist->dec_ctx->codec_type == AVMEDIA_TYPE_VIDEO ||
ist->dec_ctx->codec_type == AVMEDIA_TYPE_AUDIO) &&
pkt.dts != AV_NOPTS_VALUE && ist->next_dts == AV_NOPTS_VALUE &&
!copy_ts
Explanation:
In function "process_input" ffmpeg just handle "pts != AV_NOPTS_VALUE", but not
handle "pts == AV_NOPTS_VALUE".Now I fixed it.
if ( current_pts == AV_NOPTS_VALUE) {
current_pts = predicted_pts;
}
Reproduce:
1. Download test file http://pan.baidu.com/s/1eQsb9hK;
2. Run command below:
ffmpeg -i ./manga.01.ts -vcodec copy -bsf:v h264_mp4toannexb -hls_list_size 0
-hls_segment_filename "test_%04d.ts" playlist.m3u8
3. If the bug not fixed, the HLS slice just one; Then fix the bug, HLS' slice
is normally.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/5] ffprobe: add support for printing packet strings metadata as packet tags
On date Saturday 2015-10-24 22:42:12 +0200, Marton Balint encoded:
> ffprobe.xsd already contains the tag element.
>
> Signed-off-by: Marton Balint
> ---
> ffprobe.c | 16 +++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/ffprobe.c b/ffprobe.c
> index ac03689..f5930ae 100644
> --- a/ffprobe.c
> +++ b/ffprobe.c
> @@ -77,6 +77,7 @@ static int do_show_format_tags = 0;
> static int do_show_frame_tags = 0;
> static int do_show_program_tags = 0;
> static int do_show_stream_tags = 0;
> +static int do_show_packet_tags = 0;
>
> static int show_value_unit = 0;
> static int use_value_prefix = 0;
> @@ -135,6 +136,7 @@ typedef enum {
> SECTION_ID_LIBRARY_VERSION,
> SECTION_ID_LIBRARY_VERSIONS,
> SECTION_ID_PACKET,
> +SECTION_ID_PACKET_TAGS,
> SECTION_ID_PACKETS,
> SECTION_ID_PACKETS_AND_FRAMES,
> SECTION_ID_PACKET_SIDE_DATA_LIST,
> @@ -178,7 +180,8 @@ static struct section sections[] = {
> [SECTION_ID_LIBRARY_VERSION] ={ SECTION_ID_LIBRARY_VERSION,
> "library_version", 0, { -1 } },
> [SECTION_ID_PACKETS] ={ SECTION_ID_PACKETS, "packets",
> SECTION_FLAG_IS_ARRAY, { SECTION_ID_PACKET, -1} },
> [SECTION_ID_PACKETS_AND_FRAMES] = { SECTION_ID_PACKETS_AND_FRAMES,
> "packets_and_frames", SECTION_FLAG_IS_ARRAY, { SECTION_ID_PACKET, -1} },
> -[SECTION_ID_PACKET] = { SECTION_ID_PACKET, "packet", 0, {
> SECTION_ID_PACKET_SIDE_DATA_LIST, -1 } },
> +[SECTION_ID_PACKET] = { SECTION_ID_PACKET, "packet", 0, {
> SECTION_ID_PACKET_TAGS, SECTION_ID_PACKET_SIDE_DATA_LIST, -1 } },
> +[SECTION_ID_PACKET_TAGS] ={ SECTION_ID_PACKET_TAGS, "tags",
> SECTION_FLAG_HAS_VARIABLE_FIELDS, { -1 }, .element_name = "tag", .unique_name
> = "packet_tags" },
> [SECTION_ID_PACKET_SIDE_DATA_LIST] ={ SECTION_ID_PACKET_SIDE_DATA_LIST,
> "side_data_list", SECTION_FLAG_IS_ARRAY, { SECTION_ID_PACKET_SIDE_DATA, -1 }
> },
> [SECTION_ID_PACKET_SIDE_DATA] = { SECTION_ID_PACKET_SIDE_DATA,
> "side_data", 0, { -1 } },
> [SECTION_ID_PIXEL_FORMATS] = { SECTION_ID_PIXEL_FORMATS,
> "pixel_formats", SECTION_FLAG_IS_ARRAY, { SECTION_ID_PIXEL_FORMAT, -1 } },
> @@ -1762,6 +1765,16 @@ static void show_packet(WriterContext *w,
> AVFormatContext *fmt_ctx, AVPacket *pk
>
> if (pkt->side_data_elems) {
> int i;
> +int size;
> +const uint8_t *side_metadata;
> +
> +side_metadata = av_packet_get_side_data(pkt,
> AV_PKT_DATA_STRINGS_METADATA, &size);
> +if (side_metadata && size && do_show_packet_tags) {
> +AVDictionary *dict = NULL;
> +if (av_packet_unpack_dictionary(side_metadata, size, &dict) >= 0)
> +show_tags(w, dict, SECTION_ID_PACKET_TAGS);
> +av_dict_free(&dict);
> +}
> writer_print_section_header(w, SECTION_ID_PACKET_SIDE_DATA_LIST);
> for (i = 0; i < pkt->side_data_elems; i++) {
> AVPacketSideData *sd = &pkt->side_data[i];
> @@ -3178,6 +3191,7 @@ int main(int argc, char **argv)
> SET_DO_SHOW(FRAME_TAGS, frame_tags);
> SET_DO_SHOW(PROGRAM_TAGS, program_tags);
> SET_DO_SHOW(STREAM_TAGS, stream_tags);
> +SET_DO_SHOW(PACKET_TAGS, packet_tags);
LGTM, thanks.
--
FFmpeg = Freak and Fundamental Merciful Ponderous Eretic Guru
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 01/11] ffmpeg: use log10 instead of log()/log(10)
On Thu, Oct 29, 2015 at 8:31 PM, wm4 wrote: > On Thu, 29 Oct 2015 20:05:28 -0400 > Ganesh Ajjanagadde wrote: > >> More generally, how is this problem "easy to verify"? It may be >> included indirectly, etc. Since you seem to think it is easy, go ahead > > Indirect inclusion is IMHO not fine for such compat headers which > define standard functions on systems where they're missing. And direct > inclusion is easy to verify. > > Nobody expects that you think of everything, but here you ignored a > direct request from a reviewer. > >> >> If FFmpeg waited until verification on every single config was done, >> we would be nowhere. You may think it is not cool, I could say the >> same about many things you have posted on this mailing list as well. > > Strange that you're so awfully pedantic about C standard conformance > (so that we need dozens of patches to fix what doesn't need to be > fixed), I do not need to address your beliefs, but I do need to address outright falsehood. I normally do not address such things, but things have gone out of hand. I am tired of this, and want it on the public record the actual state of affairs. I did not even submit "dozens" of patches for C standard conformance. There was a 5-patch series for this, all available on ffmpeg-devel for those interested. Even with the 5 patches, I was clear that I am ready to drop immediately since after all they are "pedantic": https://ffmpeg.org/pipermail/ffmpeg-devel/2015-October/181972.html I knew they would likely not get much traction, and would likely be ridiculed for fixing problems that don't need fixing. I never imagined it being used in sarcasm to attack another action I took regarding FFmpeg. > but when it gets annoying for you, suddenly pushing and waiting > until it breaks is fine. How does this even make sense? > >> If Michael thought this was not cool, I will immediately take action. > > Hendrik's voice counts as much as Michael's. > ___ > 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] Idea for a better filter that reduces noise
On 10/29/2015 09:42 AM, Paul B Mahol wrote: On 10/29/15, P. van Gaans wrote: You all know the CSI episodes where they read a license plate by "enhancing" some super-grainy security footage. Nonsense, right? Well, maybe not.. If the car was parked. And it seems what I found doesn't exist yet. (but perhaps I overlooked it) If you quickly want to know what I'm on about, take a look at these images: http://members.ziggo.nl/sinaasappel/images/1_original.jpg (original) http://members.ziggo.nl/sinaasappel/images/2_40framewind.jpg (40f WIND) http://members.ziggo.nl/sinaasappel/images/3_wind_hqdn3d.jpg (comparison with hqdn3d and pp=tn) All have limited jpeg compression, but I can deliver PNG files and an XCF to experiment for yourself if desired. So what is WIND? It's what you see if you forget/fail to do motion detection. (like I did in the images) Also, Wind Is Not a Denoiser. ;-) It's a way to increase the exposure time of the camera used to shoot a movie after it's been shot. For the images, I took a 2-second somewhat grainy clip of a building with nearly no motion. I output the frames to PNG and load them as layers in The GIMP. I set the opacity for the bottom layer to 100. The layer above that 50. (100/2) Above that 33.3. (100/3) 25, 20, 16.7 and so on. Every image with noise is "wrong", some too dark and some too light. On average, they are spot-on. The advantage: improved quality and better compression. To make this into a proper useable filter, it would need to do this: 1. Deshake/stabilize the image. 2. Divide image in blocks. (e.g. 8x8 pixels) 3. Figure out it the average color of an 8x8 block is changing during the next X frames. If not, it's probably not moving and you can average the values. If it is or is about to, it should be averaged over fewer frames or not at all. Any area that is about to move will gradually pick up noise so it doesn't look too unnatural. 4. Reshake the image. Will I do this myself? Maybe, but don't hold your breath. I'm just sharing this in case somebody finds it interesting and to make sure nobody can patent it. (assuming it hasn't been done already) See atadenoise. Best regards, P. van Gaans ___ 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 Thanks Paul! I think I scrolled past atadenoise in the docs a while ago but had otherwise never heard of it. While searching for it there is little to find besides the docs and source code. (i.e. no forum threads or mentions in guides or anything) My ffmpeg doesn't even have it so I guess it's quite a new thing. I'll first compile a fresh one and get testing. I hope atadenoise also performs the deshaking internally as I described since I noticed the camera is usually not fixed, without deshaking it won't be able to match much. I still think WIND would have been a funnier name than atadenoise, but I don't complain. This will be great if it works as well as my pictures. :-) ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 01/11] ffmpeg: use log10 instead of log()/log(10)
On Thu, Oct 29, 2015 at 8:31 PM, wm4 wrote: > On Thu, 29 Oct 2015 20:05:28 -0400 > Ganesh Ajjanagadde wrote: > >> More generally, how is this problem "easy to verify"? It may be >> included indirectly, etc. Since you seem to think it is easy, go ahead > > Indirect inclusion is IMHO not fine for such compat headers which > define standard functions on systems where they're missing. And direct > inclusion is easy to verify. > > Nobody expects that you think of everything, but here you ignored a > direct request from a reviewer. > >> >> If FFmpeg waited until verification on every single config was done, >> we would be nowhere. You may think it is not cool, I could say the >> same about many things you have posted on this mailing list as well. > > Strange that you're so awfully pedantic about C standard conformance > (so that we need dozens of patches to fix what doesn't need to be > fixed), but when it gets annoying for you, suddenly pushing and waiting > until it breaks is fine. How does this even make sense? I assume an LGTM implied that the reviewer (in this case Michael) checked this issue. Seems like a misunderstanding. I did not push because it got "annoying" for me. I pushed because I got an ack from Michael. You may think whatever you want about it, e.g if you don't believe me, I am not going to try to convince you. > >> If Michael thought this was not cool, I will immediately take action. > > Hendrik's voice counts as much as Michael's. I referred to Michael as he was the reviewer for the patches. > ___ > 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 01/11] ffmpeg: use log10 instead of log()/log(10)
On Thu, Oct 29, 2015 at 8:13 PM, Hendrik Leppkes wrote: > On Fri, Oct 30, 2015 at 1:05 AM, Ganesh Ajjanagadde wrote: >> On Thu, Oct 29, 2015 at 7:50 PM, Hendrik Leppkes wrote: >>> On Fri, Oct 30, 2015 at 12:08 AM, Ganesh Ajjanagadde >>> wrote: On Thu, Oct 29, 2015 at 8:27 AM, Ronald S. Bultje wrote: > Hi, > > On Thu, Oct 29, 2015 at 8:04 AM, Michael Niedermayer > > wrote: > >> On Thu, Oct 29, 2015 at 12:19:59AM -0400, Ganesh Ajjanagadde wrote: >> > This is more concise and conveys the intent better. >> > Furthermore, it is likely more precise as well due to lack of floating >> > point division. >> > >> > Signed-off-by: Ganesh Ajjanagadde >> >> patchset tested on linux32, inux64, mingw32, ming64, qemu-mips and >> qemu-arm >> fate passes on all, i could test on actual arm&mips hw if people think >> that is needed > > > I don't think that's needed. > > Is there some way we can confirm that each of these files that is changed > includes libavutil/libm.h for the compatibility macros in case they're > lacking on the target system? pushed as is; we have until the next release or a FATE failure to check the build on such obscure configurations. I will try to keep an eye out, but please report if you find failures. >>> >>> This is really not the right way to go. You have been notified of a >>> real-world portability concern with an easy fix (verify that all files >>> you use log2/etc in include libavutil/libm.h), and you decided to wait >>> until it blows up instead? >>> Not cool. >> >> Ok. Do a grep on ffmpeg.c: line 56 answers your point. >> >> Other patches have been acked by Michael, and were pushed. >> >> More generally, how is this problem "easy to verify"? It may be >> included indirectly, etc. Since you seem to think it is easy, go ahead >> and do whatever you want. Or enlighten mortals like me who fail to see >> it is easy with your wisdom. Or if it is "easy", why don't you do it >> for the commits that I just pushed. >> >> If FFmpeg waited until verification on every single config was done, >> we would be nowhere. You may think it is not cool, I could say the >> same about many things you have posted on this mailing list as well. >> >> If Michael thought this was not cool, I will immediately take action. >> > > Its not about this patch especially, but all in the series, to which > Ronald's concerns apply equally. > If you get a review which voices concerns, these concerns need to be > addressed. Either by fixing the patch, or by convincing the reviewer > that no action is needed. Sometimes it is not possible to convince a reviewer, even when the evidence has been laid out and is right in front of them. I still recall your concerns with fabs/FFABS, where you simply did not look at my random number benchmark, or Clement's asm, or the libc link. Even if you did, your email did not reflect that. Furthermore, you did not post saying that your objection had been withdrawn. I can quote relevant links here, but doubt it is necessary. How can I guess your mind? What do you propose me to do then? Please note that this is not with respect to the patch series above, but is a general comment. > And one positive review does not overrule a > negative one - not everyone knows all possible concerns equally well. > > Pushing without addressing concerns is not OK at all. > ___ > 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]lavc/proresdec2: Fix slice_count for very high resolutions
Michael Niedermayer niedermayer.cc> writes: > > +// QT ignores the written value > > +// slice_count = AV_RB16(buf + 5); > > +slice_count = ctx->mb_height * > > ((ctx->mb_width >> log2_slice_mb_width) + > > + > > av_popcount(ctx->mb_width & (1 << log2_slice_mb_width) - 1)); > > if thats what QT does then sure LGTM Patch applied. Thank you, Carl Eugen ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 01/11] ffmpeg: use log10 instead of log()/log(10)
On Thu, 29 Oct 2015 20:05:28 -0400 Ganesh Ajjanagadde wrote: > More generally, how is this problem "easy to verify"? It may be > included indirectly, etc. Since you seem to think it is easy, go ahead Indirect inclusion is IMHO not fine for such compat headers which define standard functions on systems where they're missing. And direct inclusion is easy to verify. Nobody expects that you think of everything, but here you ignored a direct request from a reviewer. > > If FFmpeg waited until verification on every single config was done, > we would be nowhere. You may think it is not cool, I could say the > same about many things you have posted on this mailing list as well. Strange that you're so awfully pedantic about C standard conformance (so that we need dozens of patches to fix what doesn't need to be fixed), but when it gets annoying for you, suddenly pushing and waiting until it breaks is fine. How does this even make sense? > If Michael thought this was not cool, I will immediately take action. Hendrik's voice counts as much as Michael's. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]lavc/cdg: Add transparency
Michael Niedermayer niedermayer.cc> writes: > > libavcodec/cdgraphics.c |8 > > tests/fate/video.mak |2 > > tests/ref/fate/cdgraphics | 424 > i can confirm that the outputted alpha looks correct after the patch Patch applied. Carl Eugen ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 01/11] ffmpeg: use log10 instead of log()/log(10)
On Fri, Oct 30, 2015 at 12:50:46AM +0100, Hendrik Leppkes wrote: > On Fri, Oct 30, 2015 at 12:08 AM, Ganesh Ajjanagadde wrote: > > On Thu, Oct 29, 2015 at 8:27 AM, Ronald S. Bultje > > wrote: > >> Hi, > >> > >> On Thu, Oct 29, 2015 at 8:04 AM, Michael Niedermayer > >> >>> wrote: > >> > >>> On Thu, Oct 29, 2015 at 12:19:59AM -0400, Ganesh Ajjanagadde wrote: > >>> > This is more concise and conveys the intent better. > >>> > Furthermore, it is likely more precise as well due to lack of floating > >>> > point division. > >>> > > >>> > Signed-off-by: Ganesh Ajjanagadde > >>> > >>> patchset tested on linux32, inux64, mingw32, ming64, qemu-mips and > >>> qemu-arm > >>> fate passes on all, i could test on actual arm&mips hw if people think > >>> that is needed > >> > >> > >> I don't think that's needed. > >> > >> Is there some way we can confirm that each of these files that is changed > >> includes libavutil/libm.h for the compatibility macros in case they're > >> lacking on the target system? > > > > pushed as is; we have until the next release or a FATE failure to > > check the build on such obscure configurations. I will try to keep an > > eye out, but please report if you find failures. > > > > This is really not the right way to go. You have been notified of a > real-world portability concern with an easy fix (verify that all files > you use log2/etc in include libavutil/libm.h), and you decided to wait > until it blows up instead? > Not cool. I think this is partly my fault as i approved several of the patches after ronalds comment. I did check most of them for including libm.h some do through avfilter/interal.h ->avutil/internal.h->libm.h some do though avutil/common.h->avutil/internal.h->libm.h IIRC of course it is possible i forgot something and i totally didnt communicate that i did check that sorry [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB When the tyrant has disposed of foreign enemies by conquest or treaty, and there is nothing more to fear from them, then he is always stirring up some war or other, in order that the people may require a leader. -- Plato signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 01/11] ffmpeg: use log10 instead of log()/log(10)
On Fri, Oct 30, 2015 at 1:05 AM, Ganesh Ajjanagadde wrote: > On Thu, Oct 29, 2015 at 7:50 PM, Hendrik Leppkes wrote: >> On Fri, Oct 30, 2015 at 12:08 AM, Ganesh Ajjanagadde >> wrote: >>> On Thu, Oct 29, 2015 at 8:27 AM, Ronald S. Bultje >>> wrote: Hi, On Thu, Oct 29, 2015 at 8:04 AM, Michael Niedermayer wrote: > On Thu, Oct 29, 2015 at 12:19:59AM -0400, Ganesh Ajjanagadde wrote: > > This is more concise and conveys the intent better. > > Furthermore, it is likely more precise as well due to lack of floating > > point division. > > > > Signed-off-by: Ganesh Ajjanagadde > > patchset tested on linux32, inux64, mingw32, ming64, qemu-mips and > qemu-arm > fate passes on all, i could test on actual arm&mips hw if people think > that is needed I don't think that's needed. Is there some way we can confirm that each of these files that is changed includes libavutil/libm.h for the compatibility macros in case they're lacking on the target system? >>> >>> pushed as is; we have until the next release or a FATE failure to >>> check the build on such obscure configurations. I will try to keep an >>> eye out, but please report if you find failures. >>> >> >> This is really not the right way to go. You have been notified of a >> real-world portability concern with an easy fix (verify that all files >> you use log2/etc in include libavutil/libm.h), and you decided to wait >> until it blows up instead? >> Not cool. > > Ok. Do a grep on ffmpeg.c: line 56 answers your point. > > Other patches have been acked by Michael, and were pushed. > > More generally, how is this problem "easy to verify"? It may be > included indirectly, etc. Since you seem to think it is easy, go ahead > and do whatever you want. Or enlighten mortals like me who fail to see > it is easy with your wisdom. Or if it is "easy", why don't you do it > for the commits that I just pushed. > > If FFmpeg waited until verification on every single config was done, > we would be nowhere. You may think it is not cool, I could say the > same about many things you have posted on this mailing list as well. > > If Michael thought this was not cool, I will immediately take action. > Its not about this patch especially, but all in the series, to which Ronald's concerns apply equally. If you get a review which voices concerns, these concerns need to be addressed. Either by fixing the patch, or by convincing the reviewer that no action is needed. And one positive review does not overrule a negative one - not everyone knows all possible concerns equally well. Pushing without addressing concerns is not OK at all. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 01/11] ffmpeg: use log10 instead of log()/log(10)
On Thu, Oct 29, 2015 at 7:50 PM, Hendrik Leppkes wrote: > On Fri, Oct 30, 2015 at 12:08 AM, Ganesh Ajjanagadde wrote: >> On Thu, Oct 29, 2015 at 8:27 AM, Ronald S. Bultje wrote: >>> Hi, >>> >>> On Thu, Oct 29, 2015 at 8:04 AM, Michael Niedermayer >>> wrote: >>> On Thu, Oct 29, 2015 at 12:19:59AM -0400, Ganesh Ajjanagadde wrote: > This is more concise and conveys the intent better. > Furthermore, it is likely more precise as well due to lack of floating > point division. > > Signed-off-by: Ganesh Ajjanagadde patchset tested on linux32, inux64, mingw32, ming64, qemu-mips and qemu-arm fate passes on all, i could test on actual arm&mips hw if people think that is needed >>> >>> >>> I don't think that's needed. >>> >>> Is there some way we can confirm that each of these files that is changed >>> includes libavutil/libm.h for the compatibility macros in case they're >>> lacking on the target system? >> >> pushed as is; we have until the next release or a FATE failure to >> check the build on such obscure configurations. I will try to keep an >> eye out, but please report if you find failures. >> > > This is really not the right way to go. You have been notified of a > real-world portability concern with an easy fix (verify that all files > you use log2/etc in include libavutil/libm.h), and you decided to wait > until it blows up instead? > Not cool. Ok. Do a grep on ffmpeg.c: line 56 answers your point. Other patches have been acked by Michael, and were pushed. More generally, how is this problem "easy to verify"? It may be included indirectly, etc. Since you seem to think it is easy, go ahead and do whatever you want. Or enlighten mortals like me who fail to see it is easy with your wisdom. Or if it is "easy", why don't you do it for the commits that I just pushed. If FFmpeg waited until verification on every single config was done, we would be nowhere. You may think it is not cool, I could say the same about many things you have posted on this mailing list as well. If Michael thought this was not cool, I will immediately take action. > > - Hendrik > ___ > 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 01/11] ffmpeg: use log10 instead of log()/log(10)
On Fri, Oct 30, 2015 at 12:08 AM, Ganesh Ajjanagadde wrote: > On Thu, Oct 29, 2015 at 8:27 AM, Ronald S. Bultje wrote: >> Hi, >> >> On Thu, Oct 29, 2015 at 8:04 AM, Michael Niedermayer >> wrote: >> >>> On Thu, Oct 29, 2015 at 12:19:59AM -0400, Ganesh Ajjanagadde wrote: >>> > This is more concise and conveys the intent better. >>> > Furthermore, it is likely more precise as well due to lack of floating >>> > point division. >>> > >>> > Signed-off-by: Ganesh Ajjanagadde >>> >>> patchset tested on linux32, inux64, mingw32, ming64, qemu-mips and >>> qemu-arm >>> fate passes on all, i could test on actual arm&mips hw if people think >>> that is needed >> >> >> I don't think that's needed. >> >> Is there some way we can confirm that each of these files that is changed >> includes libavutil/libm.h for the compatibility macros in case they're >> lacking on the target system? > > pushed as is; we have until the next release or a FATE failure to > check the build on such obscure configurations. I will try to keep an > eye out, but please report if you find failures. > This is really not the right way to go. You have been notified of a real-world portability concern with an easy fix (verify that all files you use log2/etc in include libavutil/libm.h), and you decided to wait until it blows up instead? Not cool. - Hendrik ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil/mathematics: correct documentation for av_gcd
On Thu, Oct 29, 2015 at 4:51 PM, Michael Niedermayer
wrote:
> On Wed, Oct 28, 2015 at 10:49:08PM -0400, Ganesh Ajjanagadde wrote:
>> On Wed, Oct 28, 2015 at 10:09 AM, Ganesh Ajjanagadde
>> wrote:
>> > On Tue, Oct 27, 2015 at 8:18 PM, Ganesh Ajjanagadde
>> > wrote:
>> >> av_gcd is now always defined regardless of input. This documents this
>> >> change in the "documented API". Two benefits (closely related):
>> >> 1. The function is robust, and there is no need to worry about INT64_MIN,
>> >> etc.
>> >>
>> >> 2. Clients of av_gcd, like av_reduce, can now be made fully correct.
>> >> Currently,
>> >> av_reduce can trigger undefined behavior if e.g num is INT64_MIN due to
>> >> integer overflow in the FFABS. Furthermore, this undefined behavior is
>> >> completely undocumented, and could be a fuzzer's paradise. The FFABS was
>> >> needed in the past as
>> >> av_gcd was undefined for negative inputs. In order to make av_reduce
>> >> robust, it is essential to guarantee that av_gcd works for all int64_t.
>> >>
>> >> Signed-off-by: Ganesh Ajjanagadde
>> >> ---
>> >> libavutil/mathematics.h | 6 +++---
>> >> 1 file changed, 3 insertions(+), 3 deletions(-)
>> >>
>> >> diff --git a/libavutil/mathematics.h b/libavutil/mathematics.h
>> >> index ac94488..6fc2577 100644
>> >> --- a/libavutil/mathematics.h
>> >> +++ b/libavutil/mathematics.h
>> >> @@ -77,9 +77,9 @@ enum AVRounding {
>> >> };
>> >>
>> >> /**
>> >> - * Return the greatest common divisor of a and b.
>> >> - * If both a and b are 0 or either or both are <0 then behavior is
>> >> - * undefined.
>> >> + * Compute the greatest common divisor of a and b.
>> >> + *
>> >> + * @return gcd of a and b up to sign; if a and b are both zero returns 0
>> >> */
>> >> int64_t av_const av_gcd(int64_t a, int64_t b);
>> >>
>> >> --
>> >> 2.6.2
>> >>
>> >
>> > Patch dropped for now, undefined behavior is still possible with
>> > av_gcd: take a and b to be both INT64_MIN. Need to examine this a
>> > little more closely to make it robust without losing performance.
>>
>> Request to reconsider; patch making av_gcd more robust sent.
>
> I guess if the stricter API doesnt prevent any possibly optimizations
> then the change is a good idea
I can't think of any other clean, performant ways of solving our
issues with av_reduce etc that will depend on taking gcd of negative
values (we can't do an abs due to the fact that FFABS is unsafe on
INT64_MIN). Of course, one could have special cases for INT64_MIN etc
but that will reduce performance and even readability to some degree.
Hence, negative behavior of av_gcd must be defined, and moreover it
must not only return a value, but a mathematically correct gcd up to
sign. I leave the sign flexible in such cases to allow some wiggle
room for optimizations, and this partially addresses your point.
A low hanging optimization would come from changing everything to
uint64_t, as that would allow removal of the llabs (cost of two
branches). In fact, given the documentation currently, I wonder why
the signature was not uint64_t av_gcd(uint64_t a, uint64_t b). That
would have made work easier. However, I pesonally don't deem this
sufficient justification to create e.g an av_gcd2 with the above
signature.
As another remark, I doubt optimizing gcd further beyond simple
changes like that is very useful. It is not very critical, readability
is important, and attention may be devoted elsewhere. In fact,
although you did explain to me where optimizations could be useful at
an algorithmic level privately, I take this as an opportunity to
request for general comments on places that people think could benefit
from performance at a C/algorithmic level (NOT asm/simd stuff for me
personally): yet another ping for Clement.
That is also related to a potential GsOC/Outreachy idea (may have been
proposed already, definitely been remarked upon in the past): creation
of some kind of infrastructure to view performance vs time. For
instance, this could tie in with the current FATE, e.g one could have
a fate-perf target that does some good profiling, plots graphs, etc
that may be viewed at fate.ffmpeg.org. This could also be useful as a
public image boost, since everyone gets to see our optimizations at
work :). I don't know how to do this myself. At the risk of offending
some here, https://www.youtube.com/watch?v=kWnx6eOGVYo by Mans looked
pretty decent as a starting point on GNU/Linux.
However, you reminded me of another thing that must be documented: on
both a and b being nonnegative, we must return a nonnegative result.
Reasons:
1. A negative gcd in such cases is weird (though I won't go so far to
say it is incorrect mathematically).
2. More seriously, not doing so is an API breakage, since clients
before could have relied on this. FFmpeg itself definitely did rely on
this.
If above reasoning seems fine, I will push with an additional remark
on the nonnegativity for nonnegative arguments (satisfied by av_gcd in
its current form).
>
> [...]
> --
Re: [FFmpeg-devel] [PATCH 1/2] swscale/swscale_internal: add av_warn_unused_result
On Thu, Oct 29, 2015 at 5:50 PM, Michael Niedermayer wrote: > On Thu, Oct 15, 2015 at 09:46:08PM -0400, Ganesh Ajjanagadde wrote: >> This triggers a few warnings that need to be fixed. > > do you have a list of warnings that adds ? > Iam asking as it would make it easier to judge if they are all issues > that need fixing or if any are valid cases that do not need a return > value check May be easily obtained by running a build with a reasonably recent clang/gcc (just grep for -Wunused-result and filter out the buffersrc stuff). Will post explicitly later. > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > What does censorship reveal? It reveals fear. -- Julian Assange > > ___ > 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] avutil/mathematics: make av_gcd more robust
On Thu, Oct 29, 2015 at 4:46 PM, Michael Niedermayer wrote: > On Wed, Oct 28, 2015 at 10:48:14PM -0400, Ganesh Ajjanagadde wrote: >> This ensures that no undefined behavior is invoked, while retaining >> identical return values in all cases and at no loss of performance >> (identical asm on clang and gcc). >> Essentially, this patch exchanges undefined behavior with implementation >> defined behavior, a strict improvement. >> >> Rationale: >> 1. The ideal solution is to have the return type a uint64_t. This >> unfortunately requires an API change. >> 2. The only pathological behavior happens if both arguments are >> INT64_MIN, to the best of my knowledge. In such a case, the >> implementation defined behavior is invoked in the sense that UINT64_MAX >> is interpreted as INT64_MIN, which any reasonable implementation will >> do. In any case, any usage where both arguments are INT64_MIN is a >> fuzzer anyway. >> 3. Alternatives of checking, etc require branching and lose performance >> for no concrete gain - no client cares about av_gcd's actual value when >> both args are INT64_MIN. Even if it did, on sane platforms (e.g all the >> ones FFmpeg cares about), it produces a correct gcd, namely INT64_MIN. >> >> Signed-off-by: Ganesh Ajjanagadde >> --- >> libavutil/mathematics.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > > LGTM pushed, thanks. > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Let us carefully observe those good qualities wherein our enemies excel us > and endeavor to excel them, by avoiding what is faulty, and imitating what > is excellent in them. -- Plutarch > > ___ > 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 08/11] avfilter/avf_showvolume: use log10 instead of log()/M_LN10
On Thu, Oct 29, 2015 at 9:35 AM, Ganesh Ajjanagadde wrote: > > On Oct 29, 2015 9:26 AM, "Ronald S. Bultje" wrote: >> >> Hi, >> >> On Thu, Oct 29, 2015 at 8:47 AM, Ganesh Ajjanagadde >> wrote: >> >> > On Thu, Oct 29, 2015 at 8:22 AM, Paul B Mahol wrote: >> > > On 10/29/15, Ganesh Ajjanagadde wrote: >> > >> This is likely more precise and conveys the intent better. >> > >> >> > >> Signed-off-by: Ganesh Ajjanagadde >> > >> --- >> > >> libavfilter/avf_showvolume.c | 2 +- >> > >> 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> >> > >> diff --git a/libavfilter/avf_showvolume.c >> > >> b/libavfilter/avf_showvolume.c >> > >> index 95b5388..395375a 100644 >> > >> --- a/libavfilter/avf_showvolume.c >> > >> +++ b/libavfilter/avf_showvolume.c >> > >> @@ -197,7 +197,7 @@ static int filter_frame(AVFilterLink *inlink, >> > AVFrame >> > >> *insamples) >> > >> max = FFMAX(max, src[i]); >> > >> >> > >> max = av_clipf(max, 0, 1); >> > >> -values[VAR_VOLUME] = 20.0 * log(max) / M_LN10; >> > >> +values[VAR_VOLUME] = 20.0 * log10(max); >> > >> values[VAR_CHANNEL] = c; >> > >> color = av_expr_eval(s->c_expr, values, NULL); >> > >> >> > >> -- >> > >> 2.6.2 >> > > >> > > Have you checked which one is faster? >> > > I really have no opinion on this but gain is neglible. >> > >> > No, I have not. I suspect, but have not confirmed that log10 is >> > slightly slower: >> > >> > >> > https://stackoverflow.com/questions/10810105/explanation-wanted-log10-faster-than-log-and-log2-but-only-with-o2-and-greater >> > (janneb's comment). Keep in mind their benchmark did not include the >> > cost of a division. >> >> >> A good compiler (including yours) will merge the division into the >> multiply: >> a * func(x) / c >> is the same as: >> (a / c) * func(x) >> >> and the compiler can pre-calculate the output of a/c into a single >> constant. > > Not necessarily, floating point is not associative. pushed, thanks. > >> >> Ronald >> ___ >> 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 01/11] ffmpeg: use log10 instead of log()/log(10)
On Thu, Oct 29, 2015 at 8:27 AM, Ronald S. Bultje wrote: > Hi, > > On Thu, Oct 29, 2015 at 8:04 AM, Michael Niedermayer > wrote: > >> On Thu, Oct 29, 2015 at 12:19:59AM -0400, Ganesh Ajjanagadde wrote: >> > This is more concise and conveys the intent better. >> > Furthermore, it is likely more precise as well due to lack of floating >> > point division. >> > >> > Signed-off-by: Ganesh Ajjanagadde >> >> patchset tested on linux32, inux64, mingw32, ming64, qemu-mips and >> qemu-arm >> fate passes on all, i could test on actual arm&mips hw if people think >> that is needed > > > I don't think that's needed. > > Is there some way we can confirm that each of these files that is changed > includes libavutil/libm.h for the compatibility macros in case they're > lacking on the target system? pushed as is; we have until the next release or a FATE failure to check the build on such obscure configurations. I will try to keep an eye out, but please report if you find failures. > > Ronald > ___ > 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 10/11] avfilter/vf_ssim: use log10 instead of log()/log(10)
On Thu, Oct 29, 2015 at 2:07 PM, Ganesh Ajjanagadde wrote:
> On Thu, Oct 29, 2015 at 1:37 PM, Michael Niedermayer
> wrote:
>> On Thu, Oct 29, 2015 at 12:20:08AM -0400, Ganesh Ajjanagadde wrote:
>>> This is likely more precise and conveys the intent better.
>>>
>>> Signed-off-by: Ganesh Ajjanagadde
>>> ---
>>> libavfilter/vf_ssim.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libavfilter/vf_ssim.c b/libavfilter/vf_ssim.c
>>> index ce1e3db..6b2a8d7 100644
>>> --- a/libavfilter/vf_ssim.c
>>> +++ b/libavfilter/vf_ssim.c
>>> @@ -176,7 +176,7 @@ static float ssim_plane(SSIMDSPContext *dsp,
>>>
>>> static double ssim_db(double ssim, double weight)
>>> {
>>> -return 10 * (log(weight) / log(10) - log(weight - ssim) / log(10));
>>> +return 10 * (log10(weight) - log10(weight - ssim));
>>
>> LGTM
>>
>> note, this can be simplified further but thats maybe off topic in
>> relation to switching to log10
>
> I did note that you can rewrite as log10(weight/(weight-ssim)), but
> avoided it deliberately as I did not know what people want with
> respect to it. Since you brought it up and think it may be good, I
> will change it.
>
> I personally don't consider it too off topic, and prefer this over
> sending a separate patch for the simplification and dealing with
> another wm4 rant about it.
> Will push all later, once everything is reviewed and ok'ed.
pushed with the further simplification, thanks.
>
>>
>> [...]
>> --
>> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>
>> You can kill me, but you cannot change the truth.
>>
>> ___
>> 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 07/11] avfilter/avf_showspectrum: use log10 instead of log()/...
On Thu, Oct 29, 2015 at 5:47 PM, Michael Niedermayer wrote: > On Thu, Oct 29, 2015 at 12:20:05AM -0400, Ganesh Ajjanagadde wrote: >> This is likely more precise and conveys the intent better. >> >> Signed-off-by: Ganesh Ajjanagadde >> --- >> libavfilter/avf_showspectrum.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavfilter/avf_showspectrum.c b/libavfilter/avf_showspectrum.c >> index 936db60..7d5c438 100644 >> --- a/libavfilter/avf_showspectrum.c >> +++ b/libavfilter/avf_showspectrum.c >> @@ -386,7 +386,7 @@ static int plot_spectrum_column(AVFilterLink *inlink, >> AVFrame *insamples) >> a = cbrt(a); >> break; >> case LOG: >> -a = 1 - log(FFMAX(FFMIN(1, a), 1e-6)) / log(1e-6); // zero >> = -120dBFS >> +a = 1 + log10(FFMAX(FFMIN(1, a), 1e-6)) / 6; // zero = >> -120dBFS > > LGTM > > thx pushed, thanks > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > While the State exists there can be no freedom; when there is freedom there > will be no State. -- Vladimir Lenin > > ___ > 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 3/3] swscale: rename sws_rgb2rgb_init to ff_sws_rgb2rgb_init
On 28.10.2015 00:22, Michael Niedermayer wrote: > On Tue, Oct 27, 2015 at 11:11:09PM +0100, Andreas Cadhalpun wrote: >> It is an internal swscale function and thus should not be exported. >> >> Signed-off-by: Andreas Cadhalpun >> --- >> libswscale/colorspace-test.c | 2 +- >> libswscale/rgb2rgb.c | 2 +- >> libswscale/rgb2rgb.h | 2 +- >> libswscale/utils.c | 2 +- >> 4 files changed, 4 insertions(+), 4 deletions(-) > > probably ok Pushed. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 09/11] avfilter/vf_psnr: use log10 instead of log()/log(10)
On Thu, Oct 29, 2015 at 1:34 PM, Michael Niedermayer wrote: > On Thu, Oct 29, 2015 at 12:20:07AM -0400, Ganesh Ajjanagadde wrote: >> This is likely more precise and conveys the intent better. >> >> Signed-off-by: Ganesh Ajjanagadde >> --- >> libavfilter/vf_psnr.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > > LGTM > > thx pushed, thanks. > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Democracy is the form of government in which you can choose your dictator > > ___ > 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] avformat: stop exporting ffurl_read_complete, ffurl_seek and ffurl_size
On 27.10.2015 22:52, Andreas Cadhalpun wrote:
> On 27.10.2015 22:04, Hendrik Leppkes wrote:
>> On Tue, Oct 27, 2015 at 9:58 PM, Andreas Cadhalpun
>> wrote:
>>> They are not in public headers and not used outside of libavformat.
>>>
>>> Signed-off-by: Andreas Cadhalpun
>>> ---
>>>
>>> It's been a year since the last attempt to remove these [1].
>>> Bad enough that ffserver still uses private functions, but keeping even
>>> more in the ABI for some unnamed third party applications is just wrong.
>>>
>>> 1: https://ffmpeg.org/pipermail/ffmpeg-devel/2014-August/160951.html
>>>
>>> ---
>>> libavformat/libavformat.v | 3 ---
>>> 1 file changed, 3 deletions(-)
>>>
>>> diff --git a/libavformat/libavformat.v b/libavformat/libavformat.v
>>> index e90aef7..a00a309 100644
>>> --- a/libavformat/libavformat.v
>>> +++ b/libavformat/libavformat.v
>>> @@ -10,9 +10,6 @@ LIBAVFORMAT_$MAJOR {
>>> ffio_set_buf_size;
>>> ffurl_close;
>>> ffurl_open;
>>> -ffurl_read_complete;
>>> -ffurl_seek;
>>> -ffurl_size;
>>> ffurl_write;
>>> #those are deprecated, remove on next bump
>>> url_feof;
>>> --
>>> 2.6.1
>>>
>>
>> Fine with me.
>
> OK. I'll wait a bit to see if someone else wants to comment on this patch.
Seems not the case, so pushed now.
Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/3] swscale: rename sws_context_class to ff_sws_context_class
On 28.10.2015 00:21, Michael Niedermayer wrote: > On Tue, Oct 27, 2015 at 11:10:58PM +0100, Andreas Cadhalpun wrote: >> It is an internal swscale symbol and thus should not be exported. >> >> Signed-off-by: Andreas Cadhalpun >> --- >> libswscale/options.c | 4 ++-- >> libswscale/swscale_internal.h | 2 +- >> libswscale/utils.c| 2 +- >> 3 files changed, 4 insertions(+), 4 deletions(-) > > LGTM Pushed. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 03/11] avcodec/snowenc: use log2 instead of log() / log(2...)
On Thu, Oct 29, 2015 at 8:20 AM, Michael Niedermayer
wrote:
> On Thu, Oct 29, 2015 at 12:20:01AM -0400, Ganesh Ajjanagadde wrote:
>> This is likely more precise and conveys the intent better.
>> The expression has also been accordingly simplified.
>>
>> Signed-off-by: Ganesh Ajjanagadde
>> ---
>> libavcodec/snowenc.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/snowenc.c b/libavcodec/snowenc.c
>> index 7e8269c..fb0cd3f 100644
>> --- a/libavcodec/snowenc.c
>> +++ b/libavcodec/snowenc.c
>> @@ -1547,7 +1547,7 @@ static void calculate_visual_weight(SnowContext *s,
>> Plane *p){
>> }
>> }
>>
>> -b->qlog= (int)(log(352256.0/sqrt(error)) / log(pow(2.0,
>> 1.0/QROOT))+0.5);
>> +b->qlog= (int)(QROOT * log2(352256.0/sqrt(error)) + 0.5);
>
> LGTM
>
> thanks
pushed, thanks.
>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> While the State exists there can be no freedom; when there is freedom there
> will be no State. -- Vladimir Lenin
>
> ___
> 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 02/11] avcodec/nellymoserenc: use log2 instead of log()/M_LN2
On Thu, Oct 29, 2015 at 8:23 AM, Michael Niedermayer wrote: > On Thu, Oct 29, 2015 at 12:20:00AM -0400, Ganesh Ajjanagadde wrote: >> This is likely more precise and conveys the intent better. >> >> Signed-off-by: Ganesh Ajjanagadde >> --- >> libavcodec/nellymoserenc.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > > LGTM > > thanks pushed, thanks > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > I know you won't believe me, but the highest form of Human Excellence is > to question oneself and others. -- Socrates > > ___ > 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 04/11] avcodec/zmbvenc: use log2 instead of log()/M_LN2
On Thu, Oct 29, 2015 at 9:25 AM, Michael Niedermayer wrote: > On Thu, Oct 29, 2015 at 12:20:02AM -0400, Ganesh Ajjanagadde wrote: >> This is likely more precise and conveys the intent better. >> >> Signed-off-by: Ganesh Ajjanagadde >> --- >> libavcodec/zmbvenc.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavcodec/zmbvenc.c b/libavcodec/zmbvenc.c >> index df06e37..e832bed 100644 >> --- a/libavcodec/zmbvenc.c >> +++ b/libavcodec/zmbvenc.c >> @@ -277,7 +277,7 @@ static av_cold int encode_init(AVCodecContext *avctx) >> int lvl = 9; >> >> for(i=1; i<256; i++) >> -score_tab[i]= -i * log(i/(double)(ZMBV_BLOCK*ZMBV_BLOCK)) * >> (256/M_LN2); >> +score_tab[i]= -i * log2(i/(double)(ZMBV_BLOCK*ZMBV_BLOCK)) * 256; > > Interrestingly this changes the output from > ffmpeg -i matrixbench_mpeg2.mpg -vcodec zmbv -frames 30 -an -f framecrc - > for a few frames not too surprising, there are bit changes for many inputs when one moves from log/ to log2 etc. See e.g my reply to wm4's original comments. > > the code change looks good though pushed, thanks. > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Rewriting code that is poorly written but fully understood is good. > Rewriting code that one doesnt understand is a sign that one is less smart > then the original author, trying to rewrite it will not make it better. > > ___ > 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 05/11] avfilter/af_volume: use log10 instead of log()/M_LN10
On Thu, Oct 29, 2015 at 5:45 PM, Michael Niedermayer wrote: > On Thu, Oct 29, 2015 at 12:20:03AM -0400, Ganesh Ajjanagadde wrote: >> This is likely more precise and conveys the intent better. >> >> Signed-off-by: Ganesh Ajjanagadde >> --- >> libavfilter/af_volume.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > > LGTM > > thx pushed, thanks. > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > While the State exists there can be no freedom; when there is freedom there > will be no State. -- Vladimir Lenin > > ___ > 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] avutil/parseutils: add av_warn_unused_result
On Thu, Oct 15, 2015 at 7:52 PM, Ganesh Ajjanagadde wrote: > This triggers a few warnings that will need to be fixed - not that bad, > the current code is mostly fine. > > Signed-off-by: Ganesh Ajjanagadde > --- > libavutil/parseutils.h | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/libavutil/parseutils.h b/libavutil/parseutils.h > index e66d24b..46de518 100644 > --- a/libavutil/parseutils.h > +++ b/libavutil/parseutils.h > @@ -46,6 +46,7 @@ > * @param[in] log_ctx parent logging context > * @return >= 0 on success, a negative error code otherwise > */ > +av_warn_unused_result > int av_parse_ratio(AVRational *q, const char *str, int max, > int log_offset, void *log_ctx); > > @@ -63,6 +64,7 @@ int av_parse_ratio(AVRational *q, const char *str, int max, > * width x height or a valid video size abbreviation. > * @return >= 0 on success, a negative error code otherwise > */ > +av_warn_unused_result > int av_parse_video_size(int *width_ptr, int *height_ptr, const char *str); > > /** > @@ -74,6 +76,7 @@ int av_parse_video_size(int *width_ptr, int *height_ptr, > const char *str); > * rate_num / rate_den, a float number or a valid video rate abbreviation > * @return >= 0 on success, a negative error code otherwise > */ > +av_warn_unused_result > int av_parse_video_rate(AVRational *rate, const char *str); > > /** > @@ -95,6 +98,7 @@ int av_parse_video_rate(AVRational *rate, const char *str); > * @return >= 0 in case of success, a negative value in case of > * failure (for example if color_string cannot be parsed). > */ > +av_warn_unused_result > int av_parse_color(uint8_t *rgba_color, const char *color_string, int slen, > void *log_ctx); > > @@ -143,6 +147,7 @@ const char *av_get_known_color_name(int color_idx, const > uint8_t **rgb); > * @return >= 0 in case of success, a negative value corresponding to an > * AVERROR code otherwise > */ > +av_warn_unused_result > int av_parse_time(int64_t *timeval, const char *timestr, int duration); > > /** > -- > 2.6.1 > Pinged the wrong mail, here is the patch. Sorry. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil/pixdesc: add av_warn_unused_result to av_pix_fmt_get_chroma_sub_sample
On Thu, Oct 29, 2015 at 6:13 PM, Michael Niedermayer wrote: > On Wed, Oct 28, 2015 at 11:03:33PM -0400, Ganesh Ajjanagadde wrote: >> On Fri, Oct 16, 2015 at 5:17 PM, Ganesh Ajjanagadde wrote: >> > On Fri, Oct 16, 2015 at 11:39 AM, Michael Niedermayer >> > wrote: >> >> On Thu, Oct 15, 2015 at 07:20:30PM -0400, Ganesh Ajjanagadde wrote: >> >>> This will trigger a bunch of warnings (rightfully so). This API has been >> >>> abused, see the Doxygen comment above for what to do if the error code >> >>> is not meant to be checked. >> >>> >> >>> Signed-off-by: Ganesh Ajjanagadde >> >>> --- >> >>> libavutil/pixdesc.h | 1 + >> >>> 1 file changed, 1 insertion(+) >> >>> >> >>> diff --git a/libavutil/pixdesc.h b/libavutil/pixdesc.h >> >>> index a6056fe..975fbf2 100644 >> >>> --- a/libavutil/pixdesc.h >> >>> +++ b/libavutil/pixdesc.h >> >>> @@ -297,6 +297,7 @@ enum AVPixelFormat av_pix_fmt_desc_get_id(const >> >>> AVPixFmtDescriptor *desc); >> >>> * >> >>> * @return 0 on success, AVERROR(ENOSYS) on invalid or unknown pixel >> >>> format >> >>> */ >> >>> +av_warn_unused_result >> >>> int av_pix_fmt_get_chroma_sub_sample(enum AVPixelFormat pix_fmt, >> >>> int *h_shift, int *v_shift); >> >> >> >> if the caller knows the pixel format is valid (and non hw accel) then >> >> there is no need to check the return code >> >> in many cases this is true, like a decoder that set the pix_fmt itself >> >> it can be sure all possible values are valid >> > >> > See the Doxygen right above (this is why I pointed to it in the commit >> > message) - you are right, but for such "guaranteed safety" usage there >> > is a different API in avcodec. It seems to have been created precisely >> > for this purpose. >> >> ping; you don't find the above reasoning satisfying? > > avcodec_get_chroma_sub_sample() is part of libavcodec, > av_pix_fmt_get_chroma_sub_sample is part of libavutil > > any code not depending on libavcodec cannot use > avcodec_get_chroma_sub_sample(), an example might be filters in > libavfilter which dont otherwise depend on libavcodec Thanks for the explanation, patch dropped. > > [...] > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Freedom in capitalist society always remains about the same as it was in > ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin > > ___ > 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] avutil/parseutils: add av_warn_unused_result
On Wed, Oct 28, 2015 at 11:04:24PM -0400, Ganesh Ajjanagadde wrote: > On Thu, Oct 15, 2015 at 10:24 PM, Ganesh Ajjanagadde wrote: > > On Thu, Oct 15, 2015 at 10:22 PM, Ganesh Ajjanagadde > > wrote: > >> On Thu, Oct 15, 2015 at 10:16 PM, James Almer wrote: > >>> On 10/15/2015 10:23 PM, Ganesh Ajjanagadde wrote: > On Thu, Oct 15, 2015 at 8:06 PM, James Almer wrote: > > On 10/15/2015 8:52 PM, Ganesh Ajjanagadde wrote: > >> This triggers a few warnings that will need to be fixed - not that bad, > >> the current code is mostly fine. > >> > > > > I think i asked you before, but in any case i'll do it again. Please, > > send patchsets contained in a single thread. You're making things > > hard to track and organize on people's inboxes. > > See https://kernel.org/pub/software/scm/git/docs/git-send-email.html > > --thread, --chain-reply-to and --in-reply-to options. > > Would greatly appreciate a command/command sequence: I am on master > (which is ahead of origin/master) and contains only the avutil warning > stuff (many commits). How do I get a proper chain of emails that you > would like? > > > > > Thanks. > >>> > >>> When you send several mails with a single git send-email command, it > >>> should by default send every email after the first as a reply to the > >>> first, effectively making a single "thread". > >> > >> Sorry, but gmail smtp/send-email does not let me do this - I have > >> tried multiple patches in a single git send-email, and it closes the > >> connection. > > > > The engineers at Google must have fixed something - I tried it just > > now in the manner you described for the avdevice patches. > > ping what patch is this ping about ? i assume its not for the paragraph directly above [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I am the wisest man alive, for I know one thing, and that is that I know nothing. -- Socrates signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil/pixdesc: add av_warn_unused_result to av_pix_fmt_get_chroma_sub_sample
On Wed, Oct 28, 2015 at 11:03:33PM -0400, Ganesh Ajjanagadde wrote: > On Fri, Oct 16, 2015 at 5:17 PM, Ganesh Ajjanagadde wrote: > > On Fri, Oct 16, 2015 at 11:39 AM, Michael Niedermayer > > wrote: > >> On Thu, Oct 15, 2015 at 07:20:30PM -0400, Ganesh Ajjanagadde wrote: > >>> This will trigger a bunch of warnings (rightfully so). This API has been > >>> abused, see the Doxygen comment above for what to do if the error code > >>> is not meant to be checked. > >>> > >>> Signed-off-by: Ganesh Ajjanagadde > >>> --- > >>> libavutil/pixdesc.h | 1 + > >>> 1 file changed, 1 insertion(+) > >>> > >>> diff --git a/libavutil/pixdesc.h b/libavutil/pixdesc.h > >>> index a6056fe..975fbf2 100644 > >>> --- a/libavutil/pixdesc.h > >>> +++ b/libavutil/pixdesc.h > >>> @@ -297,6 +297,7 @@ enum AVPixelFormat av_pix_fmt_desc_get_id(const > >>> AVPixFmtDescriptor *desc); > >>> * > >>> * @return 0 on success, AVERROR(ENOSYS) on invalid or unknown pixel > >>> format > >>> */ > >>> +av_warn_unused_result > >>> int av_pix_fmt_get_chroma_sub_sample(enum AVPixelFormat pix_fmt, > >>> int *h_shift, int *v_shift); > >> > >> if the caller knows the pixel format is valid (and non hw accel) then > >> there is no need to check the return code > >> in many cases this is true, like a decoder that set the pix_fmt itself > >> it can be sure all possible values are valid > > > > See the Doxygen right above (this is why I pointed to it in the commit > > message) - you are right, but for such "guaranteed safety" usage there > > is a different API in avcodec. It seems to have been created precisely > > for this purpose. > > ping; you don't find the above reasoning satisfying? avcodec_get_chroma_sub_sample() is part of libavcodec, av_pix_fmt_get_chroma_sub_sample is part of libavutil any code not depending on libavcodec cannot use avcodec_get_chroma_sub_sample(), an example might be filters in libavfilter which dont otherwise depend on libavcodec [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Freedom in capitalist society always remains about the same as it was in ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 03/11] avdevice/sndio: add av_warn_unused_result
Hi Brad
do you have a oppinion on the change below ?
you are the author of the code
On Thu, Oct 15, 2015 at 10:22:17PM -0400, Ganesh Ajjanagadde wrote:
> This does not trigger any warnings, but adds robustness.
> Untested, as my configure does not compile this file.
>
> Signed-off-by: Ganesh Ajjanagadde
> ---
> libavdevice/sndio.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/libavdevice/sndio.h b/libavdevice/sndio.h
> index 54a5ec3..6571429 100644
> --- a/libavdevice/sndio.h
> +++ b/libavdevice/sndio.h
> @@ -42,6 +42,7 @@ typedef struct SndioData {
> int sample_rate;
> } SndioData;
>
> +av_warn_unused_result
> int ff_sndio_open(AVFormatContext *s1, int is_output, const char
> *audio_device);
> int ff_sndio_close(SndioData *s);
>
> --
> 2.6.1
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
What does censorship reveal? It reveals fear. -- Julian Assange
signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/2] swscale/swscale_internal: add av_warn_unused_result
On Thu, Oct 15, 2015 at 09:46:08PM -0400, Ganesh Ajjanagadde wrote: > This triggers a few warnings that need to be fixed. do you have a list of warnings that adds ? Iam asking as it would make it easier to judge if they are all issues that need fixing or if any are valid cases that do not need a return value check [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB What does censorship reveal? It reveals fear. -- Julian Assange signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 07/11] avfilter/avf_showspectrum: use log10 instead of log()/...
On Thu, Oct 29, 2015 at 12:20:05AM -0400, Ganesh Ajjanagadde wrote: > This is likely more precise and conveys the intent better. > > Signed-off-by: Ganesh Ajjanagadde > --- > libavfilter/avf_showspectrum.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavfilter/avf_showspectrum.c b/libavfilter/avf_showspectrum.c > index 936db60..7d5c438 100644 > --- a/libavfilter/avf_showspectrum.c > +++ b/libavfilter/avf_showspectrum.c > @@ -386,7 +386,7 @@ static int plot_spectrum_column(AVFilterLink *inlink, > AVFrame *insamples) > a = cbrt(a); > break; > case LOG: > -a = 1 - log(FFMAX(FFMIN(1, a), 1e-6)) / log(1e-6); // zero = > -120dBFS > +a = 1 + log10(FFMAX(FFMIN(1, a), 1e-6)) / 6; // zero = > -120dBFS LGTM thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB While the State exists there can be no freedom; when there is freedom there will be no State. -- Vladimir Lenin signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 05/11] avfilter/af_volume: use log10 instead of log()/M_LN10
On Thu, Oct 29, 2015 at 12:20:03AM -0400, Ganesh Ajjanagadde wrote: > This is likely more precise and conveys the intent better. > > Signed-off-by: Ganesh Ajjanagadde > --- > libavfilter/af_volume.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) LGTM thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB While the State exists there can be no freedom; when there is freedom there will be no State. -- Vladimir Lenin signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil/mathematics: correct documentation for av_gcd
On Wed, Oct 28, 2015 at 10:49:08PM -0400, Ganesh Ajjanagadde wrote:
> On Wed, Oct 28, 2015 at 10:09 AM, Ganesh Ajjanagadde
> wrote:
> > On Tue, Oct 27, 2015 at 8:18 PM, Ganesh Ajjanagadde
> > wrote:
> >> av_gcd is now always defined regardless of input. This documents this
> >> change in the "documented API". Two benefits (closely related):
> >> 1. The function is robust, and there is no need to worry about INT64_MIN,
> >> etc.
> >>
> >> 2. Clients of av_gcd, like av_reduce, can now be made fully correct.
> >> Currently,
> >> av_reduce can trigger undefined behavior if e.g num is INT64_MIN due to
> >> integer overflow in the FFABS. Furthermore, this undefined behavior is
> >> completely undocumented, and could be a fuzzer's paradise. The FFABS was
> >> needed in the past as
> >> av_gcd was undefined for negative inputs. In order to make av_reduce
> >> robust, it is essential to guarantee that av_gcd works for all int64_t.
> >>
> >> Signed-off-by: Ganesh Ajjanagadde
> >> ---
> >> libavutil/mathematics.h | 6 +++---
> >> 1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/libavutil/mathematics.h b/libavutil/mathematics.h
> >> index ac94488..6fc2577 100644
> >> --- a/libavutil/mathematics.h
> >> +++ b/libavutil/mathematics.h
> >> @@ -77,9 +77,9 @@ enum AVRounding {
> >> };
> >>
> >> /**
> >> - * Return the greatest common divisor of a and b.
> >> - * If both a and b are 0 or either or both are <0 then behavior is
> >> - * undefined.
> >> + * Compute the greatest common divisor of a and b.
> >> + *
> >> + * @return gcd of a and b up to sign; if a and b are both zero returns 0
> >> */
> >> int64_t av_const av_gcd(int64_t a, int64_t b);
> >>
> >> --
> >> 2.6.2
> >>
> >
> > Patch dropped for now, undefined behavior is still possible with
> > av_gcd: take a and b to be both INT64_MIN. Need to examine this a
> > little more closely to make it robust without losing performance.
>
> Request to reconsider; patch making av_gcd more robust sent.
I guess if the stricter API doesnt prevent any possibly optimizations
then the change is a good idea
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil/mathematics: make av_gcd more robust
On Wed, Oct 28, 2015 at 10:48:14PM -0400, Ganesh Ajjanagadde wrote: > This ensures that no undefined behavior is invoked, while retaining > identical return values in all cases and at no loss of performance > (identical asm on clang and gcc). > Essentially, this patch exchanges undefined behavior with implementation > defined behavior, a strict improvement. > > Rationale: > 1. The ideal solution is to have the return type a uint64_t. This > unfortunately requires an API change. > 2. The only pathological behavior happens if both arguments are > INT64_MIN, to the best of my knowledge. In such a case, the > implementation defined behavior is invoked in the sense that UINT64_MAX > is interpreted as INT64_MIN, which any reasonable implementation will > do. In any case, any usage where both arguments are INT64_MIN is a > fuzzer anyway. > 3. Alternatives of checking, etc require branching and lose performance > for no concrete gain - no client cares about av_gcd's actual value when > both args are INT64_MIN. Even if it did, on sane platforms (e.g all the > ones FFmpeg cares about), it produces a correct gcd, namely INT64_MIN. > > Signed-off-by: Ganesh Ajjanagadde > --- > libavutil/mathematics.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) LGTM [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Let us carefully observe those good qualities wherein our enemies excel us and endeavor to excel them, by avoiding what is faulty, and imitating what is excellent in them. -- Plutarch signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] vf_lut: Add support for RGB48 and RGBA64
On 10/25/15, Steven Robertson wrote: > Done, thanks. > probably ok > > On Tue, Oct 13, 2015 at 12:51 AM, Paul B Mahol wrote: > >> On 10/11/15, Steven Robertson wrote: >> > Thanks for taking a look! >> > >> > Steve >> > >> >> lgtm, do you need to update fate? >> ___ >> 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] avutil/opencl_internal: add av_warn_unused_result
On Wed, Oct 28, 2015 at 10:53:59PM -0400, Ganesh Ajjanagadde wrote:
> On Thu, Oct 15, 2015 at 6:24 PM, Ganesh Ajjanagadde
> wrote:
> > Signed-off-by: Ganesh Ajjanagadde
> > ---
> > libavutil/opencl_internal.h | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/libavutil/opencl_internal.h b/libavutil/opencl_internal.h
> > index dacd930..a4f5885 100644
> > --- a/libavutil/opencl_internal.h
> > +++ b/libavutil/opencl_internal.h
> > @@ -30,4 +30,5 @@ typedef struct {
> > void *ctx;
> > } FFOpenclParam;
> >
> > +av_warn_unused_result
> > int avpriv_opencl_set_parameter(FFOpenclParam *opencl_param, ...);
> > --
> > 2.6.1
> >
>
> ping
probably ok
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH][RFC] avcodec: disallow hwaccel with frame threads
Hendrik Leppkes wrote: On Thu, Oct 22, 2015 at 11:27 AM, Wang Bin wrote: VLC is using frame threading with hwaccel Then they should fix their usage, its broken by design, as explained in detail my post earlier in this thread. Seems ffmpeg cli does the same by default - OK so users can specify -threads 1 if they use -hwaccel vdpau. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 10/11] avfilter/vf_ssim: use log10 instead of log()/log(10)
On Thu, Oct 29, 2015 at 2:33 PM, Ronald S. Bultje wrote:
> Hi,
>
> On Thu, Oct 29, 2015 at 2:07 PM, Ganesh Ajjanagadde
> wrote:
>
>> On Thu, Oct 29, 2015 at 1:37 PM, Michael Niedermayer
>> wrote:
>> > On Thu, Oct 29, 2015 at 12:20:08AM -0400, Ganesh Ajjanagadde wrote:
>> >> This is likely more precise and conveys the intent better.
>> >>
>> >> Signed-off-by: Ganesh Ajjanagadde
>> >> ---
>> >> libavfilter/vf_ssim.c | 2 +-
>> >> 1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/libavfilter/vf_ssim.c b/libavfilter/vf_ssim.c
>> >> index ce1e3db..6b2a8d7 100644
>> >> --- a/libavfilter/vf_ssim.c
>> >> +++ b/libavfilter/vf_ssim.c
>> >> @@ -176,7 +176,7 @@ static float ssim_plane(SSIMDSPContext *dsp,
>> >>
>> >> static double ssim_db(double ssim, double weight)
>> >> {
>> >> -return 10 * (log(weight) / log(10) - log(weight - ssim) / log(10));
>> >> +return 10 * (log10(weight) - log10(weight - ssim));
>> >
>> > LGTM
>> >
>> > note, this can be simplified further but thats maybe off topic in
>> > relation to switching to log10
>>
>> I did note that you can rewrite as log10(weight/(weight-ssim)), but
>> avoided it deliberately as I did not know what people want with
>> respect to it. Since you brought it up and think it may be good, I
>> will change it.
>>
>> I personally don't consider it too off topic, and prefer this over
>> sending a separate patch for the simplification and dealing with
>> another wm4 rant about it.
>> Will push all later, once everything is reviewed and ok'ed.
>
>
> Now, now, let's stay friendly and professional...
I referred to it as a "rant" because that is what it was, and I want
to make it sufficiently clear that the reason patches I send often
result in a ton of noise is often not technical, but simply a
knee-jerk reaction and associated bickering over it.
I do not enjoy dealing with such things, but I can almost surely
guarantee that is what would happen if I send a separate patch for it.
I (and most people here) want work to be done at minimal noise cost,
and hence I gave my rationale for changing without sending a separate
patch.
Your point is taken though, and I will attempt to refrain from such things.
>
> Ronald
> ___
> 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 1/2] avcodec: add ADPCM AICA decoder
On Wed, Oct 28, 2015 at 10:12:02PM +0100, Paul B Mahol wrote: > Signed-off-by: Paul B Mahol > --- > libavcodec/Makefile | 1 + > libavcodec/adpcm.c | 13 + > libavcodec/allcodecs.c | 1 + > libavcodec/avcodec.h| 1 + > libavcodec/codec_desc.c | 7 +++ > 5 files changed, 23 insertions(+) LGTM thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB When the tyrant has disposed of foreign enemies by conquest or treaty, and there is nothing more to fear from them, then he is always stirring up some war or other, in order that the people may require a leader. -- Plato signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avformat: add DC STR demuxer
On Wed, Oct 28, 2015 at 10:12:03PM +0100, Paul B Mahol wrote: > Signed-off-by: Paul B Mahol > --- > libavformat/Makefile | 1 + > libavformat/allformats.c | 1 + > libavformat/dcstr.c | 79 > > 3 files changed, 81 insertions(+) > create mode 100644 libavformat/dcstr.c LGTM thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB He who knows, does not speak. He who speaks, does not know. -- Lao Tsu signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 10/11] avfilter/vf_ssim: use log10 instead of log()/log(10)
Hi,
On Thu, Oct 29, 2015 at 2:07 PM, Ganesh Ajjanagadde
wrote:
> On Thu, Oct 29, 2015 at 1:37 PM, Michael Niedermayer
> wrote:
> > On Thu, Oct 29, 2015 at 12:20:08AM -0400, Ganesh Ajjanagadde wrote:
> >> This is likely more precise and conveys the intent better.
> >>
> >> Signed-off-by: Ganesh Ajjanagadde
> >> ---
> >> libavfilter/vf_ssim.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/libavfilter/vf_ssim.c b/libavfilter/vf_ssim.c
> >> index ce1e3db..6b2a8d7 100644
> >> --- a/libavfilter/vf_ssim.c
> >> +++ b/libavfilter/vf_ssim.c
> >> @@ -176,7 +176,7 @@ static float ssim_plane(SSIMDSPContext *dsp,
> >>
> >> static double ssim_db(double ssim, double weight)
> >> {
> >> -return 10 * (log(weight) / log(10) - log(weight - ssim) / log(10));
> >> +return 10 * (log10(weight) - log10(weight - ssim));
> >
> > LGTM
> >
> > note, this can be simplified further but thats maybe off topic in
> > relation to switching to log10
>
> I did note that you can rewrite as log10(weight/(weight-ssim)), but
> avoided it deliberately as I did not know what people want with
> respect to it. Since you brought it up and think it may be good, I
> will change it.
>
> I personally don't consider it too off topic, and prefer this over
> sending a separate patch for the simplification and dealing with
> another wm4 rant about it.
> Will push all later, once everything is reviewed and ok'ed.
Now, now, let's stay friendly and professional...
Ronald
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 10/11] avfilter/vf_ssim: use log10 instead of log()/log(10)
On Thu, Oct 29, 2015 at 1:37 PM, Michael Niedermayer
wrote:
> On Thu, Oct 29, 2015 at 12:20:08AM -0400, Ganesh Ajjanagadde wrote:
>> This is likely more precise and conveys the intent better.
>>
>> Signed-off-by: Ganesh Ajjanagadde
>> ---
>> libavfilter/vf_ssim.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavfilter/vf_ssim.c b/libavfilter/vf_ssim.c
>> index ce1e3db..6b2a8d7 100644
>> --- a/libavfilter/vf_ssim.c
>> +++ b/libavfilter/vf_ssim.c
>> @@ -176,7 +176,7 @@ static float ssim_plane(SSIMDSPContext *dsp,
>>
>> static double ssim_db(double ssim, double weight)
>> {
>> -return 10 * (log(weight) / log(10) - log(weight - ssim) / log(10));
>> +return 10 * (log10(weight) - log10(weight - ssim));
>
> LGTM
>
> note, this can be simplified further but thats maybe off topic in
> relation to switching to log10
I did note that you can rewrite as log10(weight/(weight-ssim)), but
avoided it deliberately as I did not know what people want with
respect to it. Since you brought it up and think it may be good, I
will change it.
I personally don't consider it too off topic, and prefer this over
sending a separate patch for the simplification and dealing with
another wm4 rant about it.
Will push all later, once everything is reviewed and ok'ed.
>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> You can kill me, but you cannot change the truth.
>
> ___
> 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] Added QSV based VPP filter - second try
Sven Dueking nablet.com> writes:
> Please find attached my second attempt to add Intel´s
> VPP to FFMpeg. As requested, the patch includes a
> documentation file
I don't know much about how the documentation works
but why isn't this part of the filter documentation
and how are users supposed to find it?
> +AV_PIX_FMT_RGB32,
The Intel documentation for RGB4 reads:
"ARGB in that order, A channel is 8 MSBs"
Making this AV_PIX_FMT_ARGB imo
But I'm probably wrong again...
> +if (inlink->format == AV_PIX_FMT_YUV420P)
> +vpp->pVppParam->vpp.In.FourCC = MFX_FOURCC_YV12;
> +else if (inlink->format == AV_PIX_FMT_YUV422P)
> +vpp->pVppParam->vpp.In.FourCC = MFX_FOURCC_YUY2;
> +else if (inlink->format == AV_PIX_FMT_NV12)
> +vpp->pVppParam->vpp.In.FourCC = MFX_FOURCC_NV12;
> +else
> +vpp->pVppParam->vpp.In.FourCC = MFX_FOURCC_RGB4;
Did you consider to add BGR4 and ARGB16 in the future?
> +unsigned int bits_per_pixel = get_bpp(vpp->pVppParam->vpp.In.FourCC);
See below.
> +width = (unsigned int) FFALIGN(vpp->pVppParam->vpp.In.Width, 32);
> +height = (unsigned int) FFALIGN(vpp->pVppParam->vpp.In.Height, 32);
Are the casts necessary or useful?
> +vpp->in_surface = av_mallocz(sizeof(mfxFrameSurface1) *
> vpp->num_surfaces_in);
This looks wrong to me, I believe you want to allocate
num_surfaces_in pointers, not structs. (Sorry if I miss
somthing.)
> +return -1;
ENOMEM.
> +for (i = 0; i < vpp->num_surfaces_in; i++)
> +vpp->in_surface[i] = NULL;
This is unnecessary if you use mallocz() as you do.
> +for (i = 0; i < vpp->num_surfaces_in; i++) {
> +vpp->in_surface[i] = av_mallocz(sizeof(mfxFrameSurface1));
> +
> +if (!vpp->in_surface[i])
> +return -1;
ENOMEM but this leaks memory, you have to free
everything else on failure.
(same below for out_surface)
> +bits_per_pixel = 12;
Imo, remove the variable and use get_bpp() once
and "12" on the second usage.
Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 10/11] avfilter/vf_ssim: use log10 instead of log()/log(10)
On Thu, Oct 29, 2015 at 12:20:08AM -0400, Ganesh Ajjanagadde wrote:
> This is likely more precise and conveys the intent better.
>
> Signed-off-by: Ganesh Ajjanagadde
> ---
> libavfilter/vf_ssim.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavfilter/vf_ssim.c b/libavfilter/vf_ssim.c
> index ce1e3db..6b2a8d7 100644
> --- a/libavfilter/vf_ssim.c
> +++ b/libavfilter/vf_ssim.c
> @@ -176,7 +176,7 @@ static float ssim_plane(SSIMDSPContext *dsp,
>
> static double ssim_db(double ssim, double weight)
> {
> -return 10 * (log(weight) / log(10) - log(weight - ssim) / log(10));
> +return 10 * (log10(weight) - log10(weight - ssim));
LGTM
note, this can be simplified further but thats maybe off topic in
relation to switching to log10
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
You can kill me, but you cannot change the truth.
signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 09/11] avfilter/vf_psnr: use log10 instead of log()/log(10)
On Thu, Oct 29, 2015 at 12:20:07AM -0400, Ganesh Ajjanagadde wrote: > This is likely more precise and conveys the intent better. > > Signed-off-by: Ganesh Ajjanagadde > --- > libavfilter/vf_psnr.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) LGTM thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Democracy is the form of government in which you can choose your dictator signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avfilter/tremolo: fix wavetable buffer size
On Thu, Oct 29, 2015 at 12:48:05PM +0100, Paul B Mahol wrote:
> On 10/29/15, Kyle Swanson wrote:
> > Signed-off-by: Kyle Swanson
> > ---
> > libavfilter/af_tremolo.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/libavfilter/af_tremolo.c b/libavfilter/af_tremolo.c
> > index 50df2e4..572e9e3 100644
> > --- a/libavfilter/af_tremolo.c
> > +++ b/libavfilter/af_tremolo.c
> > @@ -72,7 +72,7 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *in)
> > dst += channels;
> > src += channels;
> > s->index++;
> > -if (s->index >= inlink->sample_rate)
> > +if (s->index >= inlink->sample_rate / s->freq)
> > s->index = 0;
> > }
> >
> > @@ -125,11 +125,11 @@ static int config_input(AVFilterLink *inlink)
> > const double offset = 1. - s->depth / 2.;
> > int i;
> >
> > -s->table = av_malloc_array(inlink->sample_rate, sizeof(*s->table));
> > +s->table = av_malloc_array(inlink->sample_rate / s->freq,
> > sizeof(*s->table));
> > if (!s->table)
> > return AVERROR(ENOMEM);
> >
> > -for (i = 0; i < inlink->sample_rate; i++) {
> > +for (i = 0; i < inlink->sample_rate / s->freq; i++) {
> > double env = s->freq * i / inlink->sample_rate;
> > env = sin(2 * M_PI * fmod(env + 0.25, 1.0));
> > s->table[i] = env * (1 - fabs(offset)) + offset;
> > --
> > 2.6.2
> >
> > ___
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
>
> probably ok
applied
thanks
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Good people do not need laws to tell them to act responsibly, while bad
people will find a way around the laws. -- Plato
signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Update Cookies on Setcookie playlist response
Em ter, 27 de out de 2015 às 09:28, Lucas Andrade escreveu: > Here is the patch to backport it to release/2.8. To be fixed on 2.8.2. > Anything else I should do? > > ps. Sorry, that's my first time on opensource project, I'm a little > confused that I needed to add it to the release branch. > Do I need to do anything else? It hasn't been added to release/2.8 branch yet. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avfilter/asrc_sine: fix options typos
On Thu, Oct 29, 2015 at 10:29:39AM -0500, Kyle Swanson wrote: > On Wed, Oct 28, 2015 at 3:02 PM, Michael Niedermayer > wrote: > > On Wed, Oct 28, 2015 at 07:10:24PM +0100, Nicolas George wrote: > >> Le septidi 7 brumaire, an CCXXIV, Kyle Swanson a écrit : > >> > Signed-off-by: Kyle Swanson > >> > --- > >> > libavfilter/asrc_sine.c | 4 ++-- > >> > 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> Of course ok, thanks. > > > > applied > > > > thanks > > > > [...] > > > > -- > > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > > > Complexity theory is the science of finding the exact solution to an > > approximation. Benchmarking OTOH is finding an approximation of the exact > > > > ___ > > This never got pushed. Can someone push it? done, sorry [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Breaking DRM is a little like attempting to break through a door even though the window is wide open and the only thing in the house is a bunch of things you dont want and which you would get tomorrow for free anyway signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] Added QSV based VPP filter - second try
Hi all, Please find attached my second attempt to add Intel´s VPP to FFMpeg. As requested, the patch includes a documentation file (patch checker shows some warnings here, but I am not sure if they needs to be fixed). Again, looking forward to get feedback and I am nervous ;). Thanks, Sven 0001-added-QSV-VPP-filter.patch Description: Binary data ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avfilter/asrc_sine: fix options typos
On Wed, Oct 28, 2015 at 3:02 PM, Michael Niedermayer wrote: > On Wed, Oct 28, 2015 at 07:10:24PM +0100, Nicolas George wrote: >> Le septidi 7 brumaire, an CCXXIV, Kyle Swanson a écrit : >> > Signed-off-by: Kyle Swanson >> > --- >> > libavfilter/asrc_sine.c | 4 ++-- >> > 1 file changed, 2 insertions(+), 2 deletions(-) >> >> Of course ok, thanks. > > applied > > thanks > > [...] > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Complexity theory is the science of finding the exact solution to an > approximation. Benchmarking OTOH is finding an approximation of the exact > > ___ This never got pushed. Can someone push it? ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 08/11] avfilter/avf_showvolume: use log10 instead of log()/M_LN10
On Oct 29, 2015 9:26 AM, "Ronald S. Bultje" wrote: > > Hi, > > On Thu, Oct 29, 2015 at 8:47 AM, Ganesh Ajjanagadde > wrote: > > > On Thu, Oct 29, 2015 at 8:22 AM, Paul B Mahol wrote: > > > On 10/29/15, Ganesh Ajjanagadde wrote: > > >> This is likely more precise and conveys the intent better. > > >> > > >> Signed-off-by: Ganesh Ajjanagadde > > >> --- > > >> libavfilter/avf_showvolume.c | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> diff --git a/libavfilter/avf_showvolume.c b/libavfilter/avf_showvolume.c > > >> index 95b5388..395375a 100644 > > >> --- a/libavfilter/avf_showvolume.c > > >> +++ b/libavfilter/avf_showvolume.c > > >> @@ -197,7 +197,7 @@ static int filter_frame(AVFilterLink *inlink, > > AVFrame > > >> *insamples) > > >> max = FFMAX(max, src[i]); > > >> > > >> max = av_clipf(max, 0, 1); > > >> -values[VAR_VOLUME] = 20.0 * log(max) / M_LN10; > > >> +values[VAR_VOLUME] = 20.0 * log10(max); > > >> values[VAR_CHANNEL] = c; > > >> color = av_expr_eval(s->c_expr, values, NULL); > > >> > > >> -- > > >> 2.6.2 > > > > > > Have you checked which one is faster? > > > I really have no opinion on this but gain is neglible. > > > > No, I have not. I suspect, but have not confirmed that log10 is > > slightly slower: > > > > https://stackoverflow.com/questions/10810105/explanation-wanted-log10-faster-than-log-and-log2-but-only-with-o2-and-greater > > (janneb's comment). Keep in mind their benchmark did not include the > > cost of a division. > > > A good compiler (including yours) will merge the division into the multiply: > a * func(x) / c > is the same as: > (a / c) * func(x) > > and the compiler can pre-calculate the output of a/c into a single constant. Not necessarily, floating point is not associative. > > Ronald > ___ > 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 04/11] avcodec/zmbvenc: use log2 instead of log()/M_LN2
On Thu, Oct 29, 2015 at 12:20:02AM -0400, Ganesh Ajjanagadde wrote: > This is likely more precise and conveys the intent better. > > Signed-off-by: Ganesh Ajjanagadde > --- > libavcodec/zmbvenc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavcodec/zmbvenc.c b/libavcodec/zmbvenc.c > index df06e37..e832bed 100644 > --- a/libavcodec/zmbvenc.c > +++ b/libavcodec/zmbvenc.c > @@ -277,7 +277,7 @@ static av_cold int encode_init(AVCodecContext *avctx) > int lvl = 9; > > for(i=1; i<256; i++) > -score_tab[i]= -i * log(i/(double)(ZMBV_BLOCK*ZMBV_BLOCK)) * > (256/M_LN2); > +score_tab[i]= -i * log2(i/(double)(ZMBV_BLOCK*ZMBV_BLOCK)) * 256; Interrestingly this changes the output from ffmpeg -i matrixbench_mpeg2.mpg -vcodec zmbv -frames 30 -an -f framecrc - for a few frames the code change looks good though [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Rewriting code that is poorly written but fully understood is good. Rewriting code that one doesnt understand is a sign that one is less smart then the original author, trying to rewrite it will not make it better. signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 08/11] avfilter/avf_showvolume: use log10 instead of log()/M_LN10
Hi, On Thu, Oct 29, 2015 at 8:47 AM, Ganesh Ajjanagadde wrote: > On Thu, Oct 29, 2015 at 8:22 AM, Paul B Mahol wrote: > > On 10/29/15, Ganesh Ajjanagadde wrote: > >> This is likely more precise and conveys the intent better. > >> > >> Signed-off-by: Ganesh Ajjanagadde > >> --- > >> libavfilter/avf_showvolume.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/libavfilter/avf_showvolume.c b/libavfilter/avf_showvolume.c > >> index 95b5388..395375a 100644 > >> --- a/libavfilter/avf_showvolume.c > >> +++ b/libavfilter/avf_showvolume.c > >> @@ -197,7 +197,7 @@ static int filter_frame(AVFilterLink *inlink, > AVFrame > >> *insamples) > >> max = FFMAX(max, src[i]); > >> > >> max = av_clipf(max, 0, 1); > >> -values[VAR_VOLUME] = 20.0 * log(max) / M_LN10; > >> +values[VAR_VOLUME] = 20.0 * log10(max); > >> values[VAR_CHANNEL] = c; > >> color = av_expr_eval(s->c_expr, values, NULL); > >> > >> -- > >> 2.6.2 > > > > Have you checked which one is faster? > > I really have no opinion on this but gain is neglible. > > No, I have not. I suspect, but have not confirmed that log10 is > slightly slower: > > https://stackoverflow.com/questions/10810105/explanation-wanted-log10-faster-than-log-and-log2-but-only-with-o2-and-greater > (janneb's comment). Keep in mind their benchmark did not include the > cost of a division. A good compiler (including yours) will merge the division into the multiply: a * func(x) / c is the same as: (a / c) * func(x) and the compiler can pre-calculate the output of a/c into a single constant. Ronald ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 01/11] ffmpeg: use log10 instead of log()/log(10)
On Thu, Oct 29, 2015 at 8:44 AM, Ronald S. Bultje wrote: > Hi, > > On Thu, Oct 29, 2015 at 8:32 AM, Ganesh Ajjanagadde > wrote: > >> On Thu, Oct 29, 2015 at 8:27 AM, Ronald S. Bultje >> wrote: >> > Hi, >> > >> > On Thu, Oct 29, 2015 at 8:04 AM, Michael Niedermayer >> > >> wrote: >> > >> >> On Thu, Oct 29, 2015 at 12:19:59AM -0400, Ganesh Ajjanagadde wrote: >> >> > This is more concise and conveys the intent better. >> >> > Furthermore, it is likely more precise as well due to lack of floating >> >> > point division. >> >> > >> >> > Signed-off-by: Ganesh Ajjanagadde >> >> >> >> patchset tested on linux32, inux64, mingw32, ming64, qemu-mips and >> >> qemu-arm >> >> fate passes on all, i could test on actual arm&mips hw if people think >> >> that is needed >> > >> > >> > I don't think that's needed. >> > >> > Is there some way we can confirm that each of these files that is changed >> > includes libavutil/libm.h for the compatibility macros in case they're >> > lacking on the target system? >> >> Don't know, but in case any were wondering: I assumed the availability >> simply because existing code already used log2, log10. This is also >> why even without the accuracy change, I personally consider the >> patchset an improvement due to it making FFmpeg more consistent in its >> choice of log function. > > > The risk is that some file in the dusty corners of our codebase forgets to > include libavutil/libm.h, the system does not have log2 or whatnot (which > is exactly why we have the compat wrappers in libavutil/libm.h - they are > not there because somebody felt like writing it just in case it happens - > they were written because it _did_ happen), and as a result it fails to > compile when a user on aix or msvc2010 or whatever affected system tries to > compile the next release. Thanks for clarifying. > > That would suck, and users would think ffmpeg sucks as a result. So that's > why I'm wondering if we can confirm that all affected files do include > libavutil/libm.h, so that we can be sure there's nothing to worry about. Indeed. > > Ronald > ___ > 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 08/11] avfilter/avf_showvolume: use log10 instead of log()/M_LN10
On Thu, Oct 29, 2015 at 8:22 AM, Paul B Mahol wrote: > On 10/29/15, Ganesh Ajjanagadde wrote: >> This is likely more precise and conveys the intent better. >> >> Signed-off-by: Ganesh Ajjanagadde >> --- >> libavfilter/avf_showvolume.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavfilter/avf_showvolume.c b/libavfilter/avf_showvolume.c >> index 95b5388..395375a 100644 >> --- a/libavfilter/avf_showvolume.c >> +++ b/libavfilter/avf_showvolume.c >> @@ -197,7 +197,7 @@ static int filter_frame(AVFilterLink *inlink, AVFrame >> *insamples) >> max = FFMAX(max, src[i]); >> >> max = av_clipf(max, 0, 1); >> -values[VAR_VOLUME] = 20.0 * log(max) / M_LN10; >> +values[VAR_VOLUME] = 20.0 * log10(max); >> values[VAR_CHANNEL] = c; >> color = av_expr_eval(s->c_expr, values, NULL); >> >> -- >> 2.6.2 > > Have you checked which one is faster? > I really have no opinion on this but gain is neglible. No, I have not. I suspect, but have not confirmed that log10 is slightly slower: https://stackoverflow.com/questions/10810105/explanation-wanted-log10-faster-than-log-and-log2-but-only-with-o2-and-greater (janneb's comment). Keep in mind their benchmark did not include the cost of a division. I also suspect that this is mostly an artifact of libm, see links: https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/ieee754/dbl-64/e_log10.c;hb=HEAD https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/ieee754/dbl-64/e_log2.c;hb=HEAD https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/ieee754/dbl-64/e_log.c;hb=HEAD Basically log10 uses _ieee754_log (essentially log) underneath. No one has taken the trouble of creating a different polynomial, etc for log10. I strongly suspect this is because most clients of libm anyway use log instead of log10, which is arguably rarer. Furthermore, improvements in _ieee754_log are the lowest hanging fruit for them, since due to the above, any improvements there translate into improvements in the other log functions. Lastly, work has been done on optimizing log, and due to the above architecture, log10 inherently relies on it: https://developerblog.redhat.com/2015/01/02/improving-math-performance-in-glibc/ I am pretty sure that if the libm people spent the time to optimize log10 (e.g by creating different polynomial, etc), the gap should narrow. > ___ > 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 01/11] ffmpeg: use log10 instead of log()/log(10)
Hi, On Thu, Oct 29, 2015 at 8:32 AM, Ganesh Ajjanagadde wrote: > On Thu, Oct 29, 2015 at 8:27 AM, Ronald S. Bultje > wrote: > > Hi, > > > > On Thu, Oct 29, 2015 at 8:04 AM, Michael Niedermayer > >> wrote: > > > >> On Thu, Oct 29, 2015 at 12:19:59AM -0400, Ganesh Ajjanagadde wrote: > >> > This is more concise and conveys the intent better. > >> > Furthermore, it is likely more precise as well due to lack of floating > >> > point division. > >> > > >> > Signed-off-by: Ganesh Ajjanagadde > >> > >> patchset tested on linux32, inux64, mingw32, ming64, qemu-mips and > >> qemu-arm > >> fate passes on all, i could test on actual arm&mips hw if people think > >> that is needed > > > > > > I don't think that's needed. > > > > Is there some way we can confirm that each of these files that is changed > > includes libavutil/libm.h for the compatibility macros in case they're > > lacking on the target system? > > Don't know, but in case any were wondering: I assumed the availability > simply because existing code already used log2, log10. This is also > why even without the accuracy change, I personally consider the > patchset an improvement due to it making FFmpeg more consistent in its > choice of log function. The risk is that some file in the dusty corners of our codebase forgets to include libavutil/libm.h, the system does not have log2 or whatnot (which is exactly why we have the compat wrappers in libavutil/libm.h - they are not there because somebody felt like writing it just in case it happens - they were written because it _did_ happen), and as a result it fails to compile when a user on aix or msvc2010 or whatever affected system tries to compile the next release. That would suck, and users would think ffmpeg sucks as a result. So that's why I'm wondering if we can confirm that all affected files do include libavutil/libm.h, so that we can be sure there's nothing to worry about. Ronald ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 01/11] ffmpeg: use log10 instead of log()/log(10)
On Thu, Oct 29, 2015 at 8:27 AM, Ronald S. Bultje wrote: > Hi, > > On Thu, Oct 29, 2015 at 8:04 AM, Michael Niedermayer > wrote: > >> On Thu, Oct 29, 2015 at 12:19:59AM -0400, Ganesh Ajjanagadde wrote: >> > This is more concise and conveys the intent better. >> > Furthermore, it is likely more precise as well due to lack of floating >> > point division. >> > >> > Signed-off-by: Ganesh Ajjanagadde >> >> patchset tested on linux32, inux64, mingw32, ming64, qemu-mips and >> qemu-arm >> fate passes on all, i could test on actual arm&mips hw if people think >> that is needed > > > I don't think that's needed. > > Is there some way we can confirm that each of these files that is changed > includes libavutil/libm.h for the compatibility macros in case they're > lacking on the target system? Don't know, but in case any were wondering: I assumed the availability simply because existing code already used log2, log10. This is also why even without the accuracy change, I personally consider the patchset an improvement due to it making FFmpeg more consistent in its choice of log function. > > Ronald > ___ > 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] vp9: update timestamps in ref files using multiple invisible frames.
Hi, On Thu, Oct 29, 2015 at 7:42 AM, Hendrik Leppkes wrote: > On Thu, Oct 29, 2015 at 12:40 PM, Ronald S. Bultje > wrote: > > For the 10-show-existing-frame, the source file indeed has a timestamp > > of 3 (or 100/33) for the second visible frame, so the fix appears to > > work correctly. For the other, only the timebase is fixed, but again > > appears to be correct now. > > --- > > tests/ref/fate/vp9-10-show-existing-frame | 2 +- > > tests/ref/fate/vp9-16-intra-only | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/tests/ref/fate/vp9-10-show-existing-frame > b/tests/ref/fate/vp9-10-show-existing-frame > > index 2885b00..6a2c904 100644 > > --- a/tests/ref/fate/vp9-10-show-existing-frame > > +++ b/tests/ref/fate/vp9-10-show-existing-frame > > @@ -4,7 +4,7 @@ > > #tb 0: 1/30 > > #stream#, dts,pts, duration, size, hash > > 0, 0, 0,1, 152064, > 18981342ec178e082519451062c3a67f > > -0, 1, 1,1, 152064, > 04ab9dbeac49ec31be58f6e671698e05 > > +0, 3, 3,1, 152064, > 04ab9dbeac49ec31be58f6e671698e05 > > 0, 4, 4,1, 152064, > 4ed58a0ba93a5d97a232a50c5876cda2 > > 0, 6, 6,1, 152064, > a41f00034923e56ba51a0b598acc2e3a > > 0, 7, 7,1, 152064, > 63fa55ae9535ccdf06d44cce8065dda6 > > diff --git a/tests/ref/fate/vp9-16-intra-only > b/tests/ref/fate/vp9-16-intra-only > > index 5802a53..1e8d280 100644 > > --- a/tests/ref/fate/vp9-16-intra-only > > +++ b/tests/ref/fate/vp9-16-intra-only > > @@ -1,7 +1,7 @@ > > #format: frame checksums > > #version: 1 > > #hash: MD5 > > -#tb 0: 12/359 > > +#tb 0: 1001/3 > > #stream#, dts,pts, duration, size, hash > > 0, 0, 0,1, 152064, > d57529601178948afa4818c3c8938884 > > 0, 1, 1,1, 152064, > d47e00250c45733d64af067a417bcd06 > > -- > > 2.1.2 > > > > LGTM. Pushed, and sorry for the breakage. Ronald ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 01/11] ffmpeg: use log10 instead of log()/log(10)
Hi, On Thu, Oct 29, 2015 at 8:04 AM, Michael Niedermayer wrote: > On Thu, Oct 29, 2015 at 12:19:59AM -0400, Ganesh Ajjanagadde wrote: > > This is more concise and conveys the intent better. > > Furthermore, it is likely more precise as well due to lack of floating > > point division. > > > > Signed-off-by: Ganesh Ajjanagadde > > patchset tested on linux32, inux64, mingw32, ming64, qemu-mips and > qemu-arm > fate passes on all, i could test on actual arm&mips hw if people think > that is needed I don't think that's needed. Is there some way we can confirm that each of these files that is changed includes libavutil/libm.h for the compatibility macros in case they're lacking on the target system? Ronald ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 02/11] avcodec/nellymoserenc: use log2 instead of log()/M_LN2
On Thu, Oct 29, 2015 at 12:20:00AM -0400, Ganesh Ajjanagadde wrote: > This is likely more precise and conveys the intent better. > > Signed-off-by: Ganesh Ajjanagadde > --- > libavcodec/nellymoserenc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) LGTM thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I know you won't believe me, but the highest form of Human Excellence is to question oneself and others. -- Socrates signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 08/11] avfilter/avf_showvolume: use log10 instead of log()/M_LN10
On 10/29/15, Ganesh Ajjanagadde wrote: > This is likely more precise and conveys the intent better. > > Signed-off-by: Ganesh Ajjanagadde > --- > libavfilter/avf_showvolume.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavfilter/avf_showvolume.c b/libavfilter/avf_showvolume.c > index 95b5388..395375a 100644 > --- a/libavfilter/avf_showvolume.c > +++ b/libavfilter/avf_showvolume.c > @@ -197,7 +197,7 @@ static int filter_frame(AVFilterLink *inlink, AVFrame > *insamples) > max = FFMAX(max, src[i]); > > max = av_clipf(max, 0, 1); > -values[VAR_VOLUME] = 20.0 * log(max) / M_LN10; > +values[VAR_VOLUME] = 20.0 * log10(max); > values[VAR_CHANNEL] = c; > color = av_expr_eval(s->c_expr, values, NULL); > > -- > 2.6.2 Have you checked which one is faster? I really have no opinion on this but gain is neglible. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 03/11] avcodec/snowenc: use log2 instead of log() / log(2...)
On Thu, Oct 29, 2015 at 12:20:01AM -0400, Ganesh Ajjanagadde wrote:
> This is likely more precise and conveys the intent better.
> The expression has also been accordingly simplified.
>
> Signed-off-by: Ganesh Ajjanagadde
> ---
> libavcodec/snowenc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavcodec/snowenc.c b/libavcodec/snowenc.c
> index 7e8269c..fb0cd3f 100644
> --- a/libavcodec/snowenc.c
> +++ b/libavcodec/snowenc.c
> @@ -1547,7 +1547,7 @@ static void calculate_visual_weight(SnowContext *s,
> Plane *p){
> }
> }
>
> -b->qlog= (int)(log(352256.0/sqrt(error)) / log(pow(2.0,
> 1.0/QROOT))+0.5);
> +b->qlog= (int)(QROOT * log2(352256.0/sqrt(error)) + 0.5);
LGTM
thanks
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin
signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 01/11] ffmpeg: use log10 instead of log()/log(10)
On Thu, Oct 29, 2015 at 12:19:59AM -0400, Ganesh Ajjanagadde wrote: > This is more concise and conveys the intent better. > Furthermore, it is likely more precise as well due to lack of floating > point division. > > Signed-off-by: Ganesh Ajjanagadde patchset tested on linux32, inux64, mingw32, ming64, qemu-mips and qemu-arm fate passes on all, i could test on actual arm&mips hw if people think that is needed [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB When the tyrant has disposed of foreign enemies by conquest or treaty, and there is nothing more to fear from them, then he is always stirring up some war or other, in order that the people may require a leader. -- Plato signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCHv2] all: fix enum definition for large values
Hi,
On Thu, Oct 29, 2015 at 7:55 AM, Ganesh Ajjanagadde
wrote:
> On Thu, Oct 29, 2015 at 12:29 AM, Michael Niedermayer
> wrote:
> > On Wed, Oct 28, 2015 at 07:02:42PM -0400, Ganesh Ajjanagadde wrote:
> >> On Wed, Oct 28, 2015 at 3:05 PM, Ronald S. Bultje
> wrote:
> >> > Hi,
> >> >
> >> > On Wed, Oct 28, 2015 at 2:46 PM, Ganesh Ajjanagadde <
> gajjanaga...@gmail.com>
> >> > wrote:
> >> >>
> >> >> On Wed, Oct 28, 2015 at 2:39 PM, Ronald S. Bultje <
> rsbul...@gmail.com>
> >> >> wrote:
> >> >> > Hi,
> >> >> >
> >> >> > On Wed, Oct 28, 2015 at 2:31 PM, Ganesh Ajjanagadde
> >> >> >
> >> >> > wrote:
> >> >> >>
> >> >> >> On Wed, Oct 28, 2015 at 11:38 AM, Ronald S. Bultje <
> rsbul...@gmail.com>
> >> >> >> wrote:
> >> >> >> > Hi,
> >> >> >> >
> >> >> >> > On Wed, Oct 28, 2015 at 11:00 AM, Ganesh Ajjanagadde
> >> >> >> > wrote:
> >> >> >> >>
> >> >> >> >> On Wed, Oct 28, 2015 at 10:53 AM, Ronald S. Bultje
> >> >> >> >>
> >> >> >> >> wrote:
> >> >> >> >> > Hi,
> >> >> >> >> >
> >> >> >> >> > On Wed, Oct 28, 2015 at 10:48 AM, Ganesh Ajjanagadde
> >> >> >> >> > wrote:
> >> >> >> >> >>
> >> >> >> >> >> On Wed, Oct 28, 2015 at 10:34 AM, Ronald S. Bultje
> >> >> >> >> >>
> >> >> >> >> >> wrote:
> >> >> >> >> >> > Hi,
> >> >> >> >> >> >
> >> >> >> >> >> > On Wed, Oct 28, 2015 at 8:20 AM, Ganesh Ajjanagadde
> >> >> >> >> >> >
> >> >> >> >> >> > wrote:
> >> >> >> >> >> >>
> >> >> >> >> >> >> On Wed, Oct 28, 2015 at 7:00 AM, Ronald S. Bultje
> >> >> >> >> >> >>
> >> >> >> >> >> >> wrote:
> >> >> >> >> >> >> > Hi,
> >> >> >> >> >> >> >
> >> >> >> >> >> >> > On Tue, Oct 27, 2015 at 10:58 PM, Ganesh Ajjanagadde
> >> >> >> >> >> >> > wrote:
> >> >> >> >> >> >> >>
> >> >> >> >> >> >> >> On Tue, Oct 27, 2015 at 10:41 PM, Ronald S. Bultje
> >> >> >> >> >> >> >>
> >> >> >> >> >> >> >> wrote:
> >> >> >> >> >> >> >> > Hi,
> >> >> >> >> >> >> >> >
> >> >> >> >> >> >> >> > On Tue, Oct 27, 2015 at 8:53 PM, Ganesh Ajjanagadde
> >> >> >> >> >> >> >> >
> >> >> >> >> >> >> >> > wrote:
> >> >> >> >> >> >> >> >>
> >> >> >> >> >> >> >> >> ISO C restricts enumerator values to the range of
> int.
> >> >> >> >> >> >> >> >> Thus
> >> >> >> >> >> >> >> >> (for
> >> >> >> >> >> >> >> >> instance)
> >> >> >> >> >> >> >> >> 0x8000
> >> >> >> >> >> >> >> >> unfortunately does not work, and throws a warning
> with
> >> >> >> >> >> >> >> >> -Wpedantic
> >> >> >> >> >> >> >> >> on
> >> >> >> >> >> >> >> >> clang 3.7.
> >> >> >> >> >> >> >> >>
> >> >> >> >> >> >> >> >> This fixes it by using alternative expressions that
> >> >> >> >> >> >> >> >> result
> >> >> >> >> >> >> >> >> in
> >> >> >> >> >> >> >> >> identical
> >> >> >> >> >> >> >> >> values but do not have this issue.
> >> >> >> >> >> >> >> >>
> >> >> >> >> >> >> >> >> Tested with FATE.
> >> >> >> >> >> >> >> >>
> >> >> >> >> >> >> >> >> Signed-off-by: Ganesh Ajjanagadde
> >> >> >> >> >> >> >> >>
> >> >> >> >> >> >> >> >> ---
> >> >> >> >> >> >> >> >> libavcodec/dca_syncwords.h | 26
> >> >> >> >> >> >> >> >> --
> >> >> >> >> >> >> >> >> libavformat/cinedec.c | 2 +-
> >> >> >> >> >> >> >> >> libavformat/mov_chan.c | 2 +-
> >> >> >> >> >> >> >> >> 3 files changed, 14 insertions(+), 16 deletions(-)
> >> >> >> >> >> >> >> >>
> >> >> >> >> >> >> >> >> diff --git a/libavcodec/dca_syncwords.h
> >> >> >> >> >> >> >> >> b/libavcodec/dca_syncwords.h
> >> >> >> >> >> >> >> >> index 3466b6b..6981cb8 100644
> >> >> >> >> >> >> >> >> --- a/libavcodec/dca_syncwords.h
> >> >> >> >> >> >> >> >> +++ b/libavcodec/dca_syncwords.h
> >> >> >> >> >> >> >> >> @@ -19,19 +19,17 @@
> >> >> >> >> >> >> >> >> #ifndef AVCODEC_DCA_SYNCWORDS_H
> >> >> >> >> >> >> >> >> #define AVCODEC_DCA_SYNCWORDS_H
> >> >> >> >> >> >> >> >>
> >> >> >> >> >> >> >> >> -enum DCASyncwords {
> >> >> >> >> >> >> >> >> -DCA_SYNCWORD_CORE_BE= 0x7FFE8001U,
> >> >> >> >> >> >> >> >> -DCA_SYNCWORD_CORE_LE= 0xFE7F0180U,
> >> >> >> >> >> >> >> >> -DCA_SYNCWORD_CORE_14B_BE= 0x1FFFE800U,
> >> >> >> >> >> >> >> >> -DCA_SYNCWORD_CORE_14B_LE= 0xFF1F00E8U,
> >> >> >> >> >> >> >> >> -DCA_SYNCWORD_XCH= 0x5A5A5A5AU,
> >> >> >> >> >> >> >> >> -DCA_SYNCWORD_XXCH = 0x47004A03U,
> >> >> >> >> >> >> >> >> -DCA_SYNCWORD_X96= 0x1D95F262U,
> >> >> >> >> >> >> >> >> -DCA_SYNCWORD_XBR= 0x655E315EU,
> >> >> >> >> >> >> >> >> -DCA_SYNCWORD_LBR= 0x0A801921U,
> >> >> >> >> >> >> >> >> -DCA_SYNCWORD_XLL= 0x41A29547U,
> >> >> >> >> >> >> >> >> -DCA_SYNCWORD_SUBSTREAM = 0x64582025U,
> >> >> >> >> >> >> >> >> -DCA_SYNCWORD_SUBSTREAM_CORE = 0x02B09261U,
> >> >> >> >> >> >> >> >> -};
> >> >> >> >> >> >> >> >> +#defineDCA_SYNCWORD_CORE_BE
> >> >> >> >> >> >> >> >> 0x7FFE8001U
> >> >> >> >> >> >> >> >> +#defineDCA_SYNCWORD_CORE_LE
> >> >> >> >> >> >> >> >> 0xFE7F0180U
> >> >> >> >> >> >> >> >> +#defineDCA_SYNCWORD_CORE_14B_BE
> >> >> >> >> >> >> >> >> 0x1FFFE800U
> >> >>
Re: [FFmpeg-devel] [PATCH 2/2] lavfi: add testsrc2 test source.
Le sextidi 6 brumaire, an CCXXIV, Clement Boesch a écrit : > We have testsrc and mptestsrc so... fftestsrc? or ffmire, miresrc, > colorfuzz, rainbowlicornsrc, ... or it could also just be a "ffmpeg" mode > option for testsrc so we could have all sort of mire templates (but that's > going to be inconsistent with the other smptebars & friends). > > I don't know. None of these seem really better than testsrc2, they introduce a new non-intuitive name. For now, I think I will stick to testsrc2. > BTW, while I'm at it, I think a test source generating 10-12 bits video > stream would be great for testing purpose. This mire looks like a good > candidate. This would be useful indeed. Unfortunately, all the drawutils code is really 8-bits-centric. If someone updates it to work with higher bit depths, then this source would be able to somewhat make use of it: with a little change, the diagonal gradient and the noise checkers can make use of the low order bits. For high bit depth, I suspect some kind of fractal or other mathematical pattern generator would be more suited. Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] vp9_parser: don't overwrite cached timestamps with nopts.
Hi,
On Thu, Oct 29, 2015 at 6:39 AM, Ronald S. Bultje
wrote:
> Hi,
>
> On Thu, Oct 29, 2015 at 12:45 AM, James Almer wrote:
>
>> On 10/28/2015 11:16 PM, Ronald S. Bultje wrote:
>> > Hi,
>> >
>> > On Wed, Oct 28, 2015 at 5:51 PM, wm4 wrote:
>> >
>> >> On Wed, 28 Oct 2015 16:05:49 -0400
>> >> "Ronald S. Bultje" wrote:
>> >>
>> >>> Hi,
>> >>>
>> >>> On Wed, Oct 28, 2015 at 3:44 PM, wm4 wrote:
>> >>>
>> On Wed, 28 Oct 2015 12:21:05 -0400
>> "Ronald S. Bultje" wrote:
>>
>> > ---
>> > libavcodec/vp9_parser.c | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/libavcodec/vp9_parser.c b/libavcodec/vp9_parser.c
>> > index 0437097..6713850 100644
>> > --- a/libavcodec/vp9_parser.c
>> > +++ b/libavcodec/vp9_parser.c
>> > @@ -64,7 +64,7 @@ static int parse_frame(AVCodecParserContext *ctx,
>> const uint8_t *buf, int size)
>> > if (ctx->pts == AV_NOPTS_VALUE)
>> > ctx->pts = s->pts;
>> > s->pts = AV_NOPTS_VALUE;
>> > -} else {
>> > +} else if (ctx->pts != AV_NOPTS_VALUE) {
>> > s->pts = ctx->pts;
>> > ctx->pts = AV_NOPTS_VALUE;
>> > }
>>
>> I find this a bit questionable. What is it needed for? Wouldn't it
>> repeat the previous timestamp if new packets have none?
>> >>>
>> >>>
>> >>> I don't think so. Let's first go through the problem that I'm seeing,
>> >> maybe
>> >>> you see alternate solutions. Then I'll explain (very end) why I don't
>> >> think
>> >>> this happens.
>> >>>
>> >>> So, we're assuming here (this is generally true) that all input to the
>> >>> parser has timestamps (coming from ivf/webm), and that some frames are
>> >>> "superframes" which have one invisible (alt-ref) frame (only a
>> reference,
>> >>> not an actual frame that's ever displayed) packed with the following
>> >>> visible frame. So each packet in ivf/webm leads to one visible output
>> >>> frame, but in some cases this requires decoding of multiple
>> (invisible)
>> >>> references. For frame threading purposes, we split before we send it
>> to
>> >> the
>> >>> decoder.
>> >>>
>> >>> So what you get is:
>> >>> - ivf/webm file has packet of size a with timestamp b, calls parser
>> >>> - parser notices that packet is superframe with 2 frames in it
>> >>> - we output the first (invisible) frame with no timestamp, and cache
>> the
>> >>> timestamp of the input packet
>> >>> - utils.c code calls parser again with the same input data (we told
>> it we
>> >>> didn't consume any), but the (input) timestamp is now set to nopts
>> >>> - we output the second (visible) frame with the cached timestamp on
>> the
>> >>> packet
>> >>>
>> >>> This generally makes sense, the webm/ivf indeed assume that the
>> timestamp
>> >>> only is valid for the visible frame. Invisible frames have no
>> timestamp
>> >>> associated with them since they're never displayed.
>> >>>
>> >>> So, the above code has the issue: what if there's 2 invisible frames
>> >> packed
>> >>> in a superframe (followed by the visible frame)? Right now, this
>> happens:
>> >>> - ivf/webm file has packet of size a with timestamp b, calls parser
>> >>> - parser notices that packet is superframe with 3 frames in it
>> >>> - we output the first (invisible) frame with no timestamp, and cache
>> the
>> >>> timestamp of the input packet
>> >>> - utils.c code calls parser again with the same input data (we told
>> it we
>> >>> didn't consume any), but the (input) timestamp is now set to nopts
>> >>> - we output the second (invisible) frame with no timestamp, and cache
>> the
>> >>> timestamp of the input packet (which is now set to nopts)
>> >>> - utils.c code calls parser again with the same input data (we told
>> it we
>> >>> didn't consume any), but the (input) timestamp is now set to nopts
>> >>> - we output the third (visible) frame with the cached timestamp on the
>> >>> packet, which was nopts
>> >>>
>> >>> The last 3 are wrong; we should only cache timestamps if there is any
>> to
>> >> be
>> >>> cached, we should not override the cached timestamp with a new nopts
>> >> value,
>> >>> at least not in this particular case.
>> >>>
>> >>> --
>> >>> very end
>> >>> --
>> >>>
>> >>> Ok, so what about your point: can we output the same timestamp twice?
>> I
>> >>> don't think so, because as soon as we output the cached timestamp, we
>> >> reset
>> >>> the value of the cached timestamp to nopts, so if we were to reuse the
>> >>> cached timestamp, it would be nopts and the output data would have no
>> >>> timestamp.
>> >>>
>> >>> (Hope that wasn't too detailed.)
>> >>
>> >> Thanks for the explanations. I didn't know there could be more than 1
>> >> super frame, and I see how the new logic works and doesn't duplicate
>> >> timestamps.
>> >>
>> >> Patch looks good with me then.
>> >
>> >
>> > TY, pushed.
>> >
>> > Ronald
>>
>> Did you forget to update the ref files for
>> fate-vp9-10-show-existing-frame
Re: [FFmpeg-devel] [PATCHv2] all: fix enum definition for large values
On Thu, Oct 29, 2015 at 12:29 AM, Michael Niedermayer
wrote:
> On Wed, Oct 28, 2015 at 07:02:42PM -0400, Ganesh Ajjanagadde wrote:
>> On Wed, Oct 28, 2015 at 3:05 PM, Ronald S. Bultje wrote:
>> > Hi,
>> >
>> > On Wed, Oct 28, 2015 at 2:46 PM, Ganesh Ajjanagadde
>> >
>> > wrote:
>> >>
>> >> On Wed, Oct 28, 2015 at 2:39 PM, Ronald S. Bultje
>> >> wrote:
>> >> > Hi,
>> >> >
>> >> > On Wed, Oct 28, 2015 at 2:31 PM, Ganesh Ajjanagadde
>> >> >
>> >> > wrote:
>> >> >>
>> >> >> On Wed, Oct 28, 2015 at 11:38 AM, Ronald S. Bultje
>> >> >> wrote:
>> >> >> > Hi,
>> >> >> >
>> >> >> > On Wed, Oct 28, 2015 at 11:00 AM, Ganesh Ajjanagadde
>> >> >> > wrote:
>> >> >> >>
>> >> >> >> On Wed, Oct 28, 2015 at 10:53 AM, Ronald S. Bultje
>> >> >> >>
>> >> >> >> wrote:
>> >> >> >> > Hi,
>> >> >> >> >
>> >> >> >> > On Wed, Oct 28, 2015 at 10:48 AM, Ganesh Ajjanagadde
>> >> >> >> > wrote:
>> >> >> >> >>
>> >> >> >> >> On Wed, Oct 28, 2015 at 10:34 AM, Ronald S. Bultje
>> >> >> >> >>
>> >> >> >> >> wrote:
>> >> >> >> >> > Hi,
>> >> >> >> >> >
>> >> >> >> >> > On Wed, Oct 28, 2015 at 8:20 AM, Ganesh Ajjanagadde
>> >> >> >> >> >
>> >> >> >> >> > wrote:
>> >> >> >> >> >>
>> >> >> >> >> >> On Wed, Oct 28, 2015 at 7:00 AM, Ronald S. Bultje
>> >> >> >> >> >>
>> >> >> >> >> >> wrote:
>> >> >> >> >> >> > Hi,
>> >> >> >> >> >> >
>> >> >> >> >> >> > On Tue, Oct 27, 2015 at 10:58 PM, Ganesh Ajjanagadde
>> >> >> >> >> >> > wrote:
>> >> >> >> >> >> >>
>> >> >> >> >> >> >> On Tue, Oct 27, 2015 at 10:41 PM, Ronald S. Bultje
>> >> >> >> >> >> >>
>> >> >> >> >> >> >> wrote:
>> >> >> >> >> >> >> > Hi,
>> >> >> >> >> >> >> >
>> >> >> >> >> >> >> > On Tue, Oct 27, 2015 at 8:53 PM, Ganesh Ajjanagadde
>> >> >> >> >> >> >> >
>> >> >> >> >> >> >> > wrote:
>> >> >> >> >> >> >> >>
>> >> >> >> >> >> >> >> ISO C restricts enumerator values to the range of int.
>> >> >> >> >> >> >> >> Thus
>> >> >> >> >> >> >> >> (for
>> >> >> >> >> >> >> >> instance)
>> >> >> >> >> >> >> >> 0x8000
>> >> >> >> >> >> >> >> unfortunately does not work, and throws a warning with
>> >> >> >> >> >> >> >> -Wpedantic
>> >> >> >> >> >> >> >> on
>> >> >> >> >> >> >> >> clang 3.7.
>> >> >> >> >> >> >> >>
>> >> >> >> >> >> >> >> This fixes it by using alternative expressions that
>> >> >> >> >> >> >> >> result
>> >> >> >> >> >> >> >> in
>> >> >> >> >> >> >> >> identical
>> >> >> >> >> >> >> >> values but do not have this issue.
>> >> >> >> >> >> >> >>
>> >> >> >> >> >> >> >> Tested with FATE.
>> >> >> >> >> >> >> >>
>> >> >> >> >> >> >> >> Signed-off-by: Ganesh Ajjanagadde
>> >> >> >> >> >> >> >>
>> >> >> >> >> >> >> >> ---
>> >> >> >> >> >> >> >> libavcodec/dca_syncwords.h | 26
>> >> >> >> >> >> >> >> --
>> >> >> >> >> >> >> >> libavformat/cinedec.c | 2 +-
>> >> >> >> >> >> >> >> libavformat/mov_chan.c | 2 +-
>> >> >> >> >> >> >> >> 3 files changed, 14 insertions(+), 16 deletions(-)
>> >> >> >> >> >> >> >>
>> >> >> >> >> >> >> >> diff --git a/libavcodec/dca_syncwords.h
>> >> >> >> >> >> >> >> b/libavcodec/dca_syncwords.h
>> >> >> >> >> >> >> >> index 3466b6b..6981cb8 100644
>> >> >> >> >> >> >> >> --- a/libavcodec/dca_syncwords.h
>> >> >> >> >> >> >> >> +++ b/libavcodec/dca_syncwords.h
>> >> >> >> >> >> >> >> @@ -19,19 +19,17 @@
>> >> >> >> >> >> >> >> #ifndef AVCODEC_DCA_SYNCWORDS_H
>> >> >> >> >> >> >> >> #define AVCODEC_DCA_SYNCWORDS_H
>> >> >> >> >> >> >> >>
>> >> >> >> >> >> >> >> -enum DCASyncwords {
>> >> >> >> >> >> >> >> -DCA_SYNCWORD_CORE_BE= 0x7FFE8001U,
>> >> >> >> >> >> >> >> -DCA_SYNCWORD_CORE_LE= 0xFE7F0180U,
>> >> >> >> >> >> >> >> -DCA_SYNCWORD_CORE_14B_BE= 0x1FFFE800U,
>> >> >> >> >> >> >> >> -DCA_SYNCWORD_CORE_14B_LE= 0xFF1F00E8U,
>> >> >> >> >> >> >> >> -DCA_SYNCWORD_XCH= 0x5A5A5A5AU,
>> >> >> >> >> >> >> >> -DCA_SYNCWORD_XXCH = 0x47004A03U,
>> >> >> >> >> >> >> >> -DCA_SYNCWORD_X96= 0x1D95F262U,
>> >> >> >> >> >> >> >> -DCA_SYNCWORD_XBR= 0x655E315EU,
>> >> >> >> >> >> >> >> -DCA_SYNCWORD_LBR= 0x0A801921U,
>> >> >> >> >> >> >> >> -DCA_SYNCWORD_XLL= 0x41A29547U,
>> >> >> >> >> >> >> >> -DCA_SYNCWORD_SUBSTREAM = 0x64582025U,
>> >> >> >> >> >> >> >> -DCA_SYNCWORD_SUBSTREAM_CORE = 0x02B09261U,
>> >> >> >> >> >> >> >> -};
>> >> >> >> >> >> >> >> +#defineDCA_SYNCWORD_CORE_BE
>> >> >> >> >> >> >> >> 0x7FFE8001U
>> >> >> >> >> >> >> >> +#defineDCA_SYNCWORD_CORE_LE
>> >> >> >> >> >> >> >> 0xFE7F0180U
>> >> >> >> >> >> >> >> +#defineDCA_SYNCWORD_CORE_14B_BE
>> >> >> >> >> >> >> >> 0x1FFFE800U
>> >> >> >> >> >> >> >> +#defineDCA_SYNCWORD_CORE_14B_LE
>> >> >> >> >> >> >> >> 0xFF1F00E8U
>> >> >> >> >> >> >> >> +#defineDCA_SYNCWORD_XCH
>> >> >> >> >> >> >> >> 0x5A5A5A5AU
>> >> >> >> >> >> >> >> +#defineDCA_SYNCWORD_XXCH
>> >> >> >> >> >> >> >> 0x47004A03U
>> >> >> >> >> >> >> >> +#defineDCA_SYNCWORD_X96
>> >> >> >> >> >> >> >> 0x1D95F262U
>> >> >> >
Re: [FFmpeg-devel] [PATCH 01/11] ffmpeg: use log10 instead of log()/log(10)
On Thu, Oct 29, 2015 at 7:36 AM, wm4 wrote: > On Thu, 29 Oct 2015 07:27:20 -0400 > Ganesh Ajjanagadde wrote: > >> On Thu, Oct 29, 2015 at 5:43 AM, wm4 wrote: >> > On Thu, 29 Oct 2015 00:19:59 -0400 >> > Ganesh Ajjanagadde wrote: >> > >> >> This is more concise and conveys the intent better. >> >> Furthermore, it is likely more precise as well due to lack of floating >> >> point division. >> >> >> >> Signed-off-by: Ganesh Ajjanagadde >> >> --- >> > >> > These patches are all pretty similar. And likely tedious to check for >> > correctness. So low gain, while some potential for regressions. >> >> In the time it took you to write that comment, you could have easily >> reviewed a couple. log10 is already being used in the codebase, why >> not make it consistent and also make it more precise? >> >> There is a good reason why libc has a log10 function. >> >> Maybe you don't care about such things, but I do: it is (roughly) >> analogous to using double instead of float for filters/resampling etc >> - noise floor should not be increased unless there is a clear benefit. >> Here there is none from using log as opposed to log10. > > Not going to play this superficially-review-mass-patches-with-little-to- > none-benefit-just-so-a-single-developer-doesn't-get-annoyed-and-then-deal- > with-regression-fallout game. > > You're giving the ML more patches to review than it can deal with it, so > my reaction is to flat out reject such patch-spam for things which seem to > have little benefit and are not 100% trivial. How? I send fewer patches than many other developers here. > > (This reminds me of mass cosmetics from Libav...) Comparing this with cosmetics is not accurate - these are NOT cosmetics. They affect the bit accuracy of the results. To prove to the likes of wm4 that this is not cosmetic, here are some simple results: computing 10*log10(x) vs 10*log10(x)/log(10.0), top is first, bottom is second expr: x = 1e-12: -120.000 -119.986 x = DBL_MIN: -3076.526555685887615 -3076.526555685887161 x = 5*1e32: 326.989700043360187 326.989700043360131 These are differences obtained within the first 15 digits. I am pretty confident that by finding the worst case, one can get even starker differences. > ___ > 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] avfilter/tremolo: fix wavetable buffer size
On 10/29/15, Kyle Swanson wrote:
> Signed-off-by: Kyle Swanson
> ---
> libavfilter/af_tremolo.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/libavfilter/af_tremolo.c b/libavfilter/af_tremolo.c
> index 50df2e4..572e9e3 100644
> --- a/libavfilter/af_tremolo.c
> +++ b/libavfilter/af_tremolo.c
> @@ -72,7 +72,7 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *in)
> dst += channels;
> src += channels;
> s->index++;
> -if (s->index >= inlink->sample_rate)
> +if (s->index >= inlink->sample_rate / s->freq)
> s->index = 0;
> }
>
> @@ -125,11 +125,11 @@ static int config_input(AVFilterLink *inlink)
> const double offset = 1. - s->depth / 2.;
> int i;
>
> -s->table = av_malloc_array(inlink->sample_rate, sizeof(*s->table));
> +s->table = av_malloc_array(inlink->sample_rate / s->freq,
> sizeof(*s->table));
> if (!s->table)
> return AVERROR(ENOMEM);
>
> -for (i = 0; i < inlink->sample_rate; i++) {
> +for (i = 0; i < inlink->sample_rate / s->freq; i++) {
> double env = s->freq * i / inlink->sample_rate;
> env = sin(2 * M_PI * fmod(env + 0.25, 1.0));
> s->table[i] = env * (1 - fabs(offset)) + offset;
> --
> 2.6.2
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
probably ok
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] vp9: update timestamps in ref files using multiple invisible frames.
On Thu, Oct 29, 2015 at 12:40 PM, Ronald S. Bultje wrote: > For the 10-show-existing-frame, the source file indeed has a timestamp > of 3 (or 100/33) for the second visible frame, so the fix appears to > work correctly. For the other, only the timebase is fixed, but again > appears to be correct now. > --- > tests/ref/fate/vp9-10-show-existing-frame | 2 +- > tests/ref/fate/vp9-16-intra-only | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tests/ref/fate/vp9-10-show-existing-frame > b/tests/ref/fate/vp9-10-show-existing-frame > index 2885b00..6a2c904 100644 > --- a/tests/ref/fate/vp9-10-show-existing-frame > +++ b/tests/ref/fate/vp9-10-show-existing-frame > @@ -4,7 +4,7 @@ > #tb 0: 1/30 > #stream#, dts,pts, duration, size, hash > 0, 0, 0,1, 152064, > 18981342ec178e082519451062c3a67f > -0, 1, 1,1, 152064, > 04ab9dbeac49ec31be58f6e671698e05 > +0, 3, 3,1, 152064, > 04ab9dbeac49ec31be58f6e671698e05 > 0, 4, 4,1, 152064, > 4ed58a0ba93a5d97a232a50c5876cda2 > 0, 6, 6,1, 152064, > a41f00034923e56ba51a0b598acc2e3a > 0, 7, 7,1, 152064, > 63fa55ae9535ccdf06d44cce8065dda6 > diff --git a/tests/ref/fate/vp9-16-intra-only > b/tests/ref/fate/vp9-16-intra-only > index 5802a53..1e8d280 100644 > --- a/tests/ref/fate/vp9-16-intra-only > +++ b/tests/ref/fate/vp9-16-intra-only > @@ -1,7 +1,7 @@ > #format: frame checksums > #version: 1 > #hash: MD5 > -#tb 0: 12/359 > +#tb 0: 1001/3 > #stream#, dts,pts, duration, size, hash > 0, 0, 0,1, 152064, > d57529601178948afa4818c3c8938884 > 0, 1, 1,1, 152064, > d47e00250c45733d64af067a417bcd06 > -- > 2.1.2 > LGTM. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] vp9: update timestamps in ref files using multiple invisible frames.
For the 10-show-existing-frame, the source file indeed has a timestamp of 3 (or 100/33) for the second visible frame, so the fix appears to work correctly. For the other, only the timebase is fixed, but again appears to be correct now. --- tests/ref/fate/vp9-10-show-existing-frame | 2 +- tests/ref/fate/vp9-16-intra-only | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/ref/fate/vp9-10-show-existing-frame b/tests/ref/fate/vp9-10-show-existing-frame index 2885b00..6a2c904 100644 --- a/tests/ref/fate/vp9-10-show-existing-frame +++ b/tests/ref/fate/vp9-10-show-existing-frame @@ -4,7 +4,7 @@ #tb 0: 1/30 #stream#, dts,pts, duration, size, hash 0, 0, 0,1, 152064, 18981342ec178e082519451062c3a67f -0, 1, 1,1, 152064, 04ab9dbeac49ec31be58f6e671698e05 +0, 3, 3,1, 152064, 04ab9dbeac49ec31be58f6e671698e05 0, 4, 4,1, 152064, 4ed58a0ba93a5d97a232a50c5876cda2 0, 6, 6,1, 152064, a41f00034923e56ba51a0b598acc2e3a 0, 7, 7,1, 152064, 63fa55ae9535ccdf06d44cce8065dda6 diff --git a/tests/ref/fate/vp9-16-intra-only b/tests/ref/fate/vp9-16-intra-only index 5802a53..1e8d280 100644 --- a/tests/ref/fate/vp9-16-intra-only +++ b/tests/ref/fate/vp9-16-intra-only @@ -1,7 +1,7 @@ #format: frame checksums #version: 1 #hash: MD5 -#tb 0: 12/359 +#tb 0: 1001/3 #stream#, dts,pts, duration, size, hash 0, 0, 0,1, 152064, d57529601178948afa4818c3c8938884 0, 1, 1,1, 152064, d47e00250c45733d64af067a417bcd06 -- 2.1.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 5/8] tools/crypto_bench: add AES-CBC modes
L'octidi 8 brumaire, an CCXXIV, Rodger Combs a écrit :
> Take a look at some results; CBC is significantly slower than CTR in both
> libcrypto's and our AES code. There's some hardware limitation it hits.
Yes, that is to be expected. But I would expect something like this:
time(AES-X-CBC) - time(AES-X-ECB) = time(AES-Y-CBC) - time(AES-Y-ECB)
for X and Y in { 128, 192, 256 }. In other words, I expect the extra time
added by CBC to be independent from the key size.
But of course, having a benchmark to check that it is true is good.
So patch still ok, thanks.
Regards,
--
Nicolas George
signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 01/11] ffmpeg: use log10 instead of log()/log(10)
On Thu, 29 Oct 2015 07:27:20 -0400 Ganesh Ajjanagadde wrote: > On Thu, Oct 29, 2015 at 5:43 AM, wm4 wrote: > > On Thu, 29 Oct 2015 00:19:59 -0400 > > Ganesh Ajjanagadde wrote: > > > >> This is more concise and conveys the intent better. > >> Furthermore, it is likely more precise as well due to lack of floating > >> point division. > >> > >> Signed-off-by: Ganesh Ajjanagadde > >> --- > > > > These patches are all pretty similar. And likely tedious to check for > > correctness. So low gain, while some potential for regressions. > > In the time it took you to write that comment, you could have easily > reviewed a couple. log10 is already being used in the codebase, why > not make it consistent and also make it more precise? > > There is a good reason why libc has a log10 function. > > Maybe you don't care about such things, but I do: it is (roughly) > analogous to using double instead of float for filters/resampling etc > - noise floor should not be increased unless there is a clear benefit. > Here there is none from using log as opposed to log10. Not going to play this superficially-review-mass-patches-with-little-to- none-benefit-just-so-a-single-developer-doesn't-get-annoyed-and-then-deal- with-regression-fallout game. You're giving the ML more patches to review than it can deal with it, so my reaction is to flat out reject such patch-spam for things which seem to have little benefit and are not 100% trivial. (This reminds me of mass cosmetics from Libav...) ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] fate: add decimate test.
Le quintidi 5 brumaire, an CCXXIV, Michael Niedermayer a écrit : > seems failing as 32bit on 64bit x86 linux (works on 64bit) > > --- ffmpeg/tests/ref/fate/filter-decimate-simple 2015-10-25 > 23:19:29.618103426 +0100 > +++ tests/data/fate/filter-decimate-simple 2015-10-26 00:02:08.838157343 > +0100 > @@ -2,17 +2,17 @@ > 0, 1, 1,1, 115200, 0x5e0f54f3 > 0, 2, 2,1, 115200, 0xd5c25c93 > 0, 3, 3,1, 115200, 0x372c655c > -0, 5, 5,1, 115200, 0xedbaed7f > +0, 4, 4,1, 115200, 0xedbaed7f > 0, 6, 6,1, 115200, 0x8b2a40a6 > 0, 7, 7,1, 115200, 0x4e0547fe > -0, 9, 9,1, 115200, 0xca5a3761 > +0, 8, 8,1, 115200, 0xca5a3761 > 0, 10, 10,1, 115200, 0xe7562c6f > 0, 11, 11,1, 115200, 0x709f3960 > -0, 13, 13,1, 115200, 0x2f76d8f2 > +0, 12, 12,1, 115200, 0x2f76d8f2 > 0, 14, 14,1, 115200, 0x72045ce1 > 0, 15, 15,1, 115200, 0xa6b36e8e > -0, 17, 17,1, 115200, 0x275a0a3a > +0, 16, 16,1, 115200, 0x275a0a3a > 0, 18, 18,1, 115200, 0x87f04127 > 0, 19, 19,1, 115200, 0x442d4f33 > -0, 21, 21,1, 115200, 0x8e892c86 > +0, 20, 20,1, 115200, 0x8e892c86 > 0, 22, 22,1, 115200, 0xb214258f > Test filter-decimate-simple failed. Look at > tests/data/fate/filter-decimate-simple.err for details. > make: *** [fate-filter-decimate-simple] Error 1 Thanks for catching this. It selects the same frame, but gives it a different timestamp. I just noticed the timestamp computations are made with doubles, and does not take the input frame timestamps into account. I would say that this filter is broken in several ways, there is no need for testing it before it is fixed. Patch dropped. Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil/opt: add av_warn_unused_result
On Thu, Oct 29, 2015 at 5:23 AM, wm4 wrote: > On Thu, 29 Oct 2015 09:17:16 +0100 > Hendrik Leppkes wrote: > >> On Thu, Oct 29, 2015 at 3:51 AM, Ganesh Ajjanagadde >> wrote: >> > On Thu, Oct 15, 2015 at 6:12 PM, Ganesh Ajjanagadde >> > wrote: >> >> Some warnings do get triggered that will need to be fixed. >> >> >> >> Signed-off-by: Ganesh Ajjanagadde >> >> --- >> >> libavutil/opt.h | 2 ++ >> >> 1 file changed, 2 insertions(+) >> >> >> >> diff --git a/libavutil/opt.h b/libavutil/opt.h >> >> index 753434d..5abe963 100644 >> >> --- a/libavutil/opt.h >> >> +++ b/libavutil/opt.h >> >> @@ -414,6 +414,7 @@ void av_opt_set_defaults2(void *s, int mask, int >> >> flags); >> >> * the error code issued by av_opt_set() if a key/value pair >> >> * cannot be set >> >> */ >> >> +av_warn_unused_result >> >> int av_set_options_string(void *ctx, const char *opts, >> >>const char *key_val_sep, const char >> >> *pairs_sep); >> >> >> >> @@ -444,6 +445,7 @@ int av_set_options_string(void *ctx, const char *opts, >> >> * Separators must use characters distinct from option names and from >> >> each >> >> * other. >> >> */ >> >> +av_warn_unused_result >> >> int av_opt_set_from_string(void *ctx, const char *opts, >> >> const char *const *shorthand, >> >> const char *key_val_sep, const char >> >> *pairs_sep); >> >> -- >> >> 2.6.1 >> >> >> > >> > ping >> >> I would argue that failure from these functions is hardly critical, >> ie. it can fail if an option is not found, and enforcing checks here >> seems a bit too aggressive to me. > > +1 > > You might often set options on e.g. a codec generically, without caring > much whether it fails or not. This is because different codecs have > different options, and those which lack a certain option will work > without setting it. (Just as an example.) Thanks for examining, patch dropped. > ___ > 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 5/8] tools/crypto_bench: add AES-CBC modes
Take a look at some results; CBC is significantly slower than CTR in both libcrypto's and our AES code. There's some hardware limitation it hits. > On Oct 29, 2015, at 06:13, Nicolas George wrote: > > Le septidi 7 brumaire, an CCXXIV, Rodger Combs a écrit : >> --- >> tools/crypto_bench.c | 140 >> +-- >> 1 file changed, 137 insertions(+), 3 deletions(-) > > Should be ok. Not sure all the modes are really needed since the operation > mode is quite orthogonal to the block cipher itself, but it does not matter > much. > > Regards, > > -- > Nicolas George > ___ > 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 11/11] avutil/rational: use frexp rather than ad-hoc log to get floating point exponent
On Thu, Oct 29, 2015 at 12:20 AM, Ganesh Ajjanagadde
wrote:
> This simplifies and cleans up the code.
> Furthermore, it is much faster due to absence of the slow log computation.
>
> Signed-off-by: Ganesh Ajjanagadde
> ---
> libavutil/rational.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/libavutil/rational.c b/libavutil/rational.c
> index 21d2bb7..81a9402 100644
> --- a/libavutil/rational.c
> +++ b/libavutil/rational.c
> @@ -106,14 +106,14 @@ AVRational av_sub_q(AVRational b, AVRational c)
> AVRational av_d2q(double d, int max)
> {
> AVRational a;
> -#define LOG2 0.69314718055994530941723212145817656807550013436025
> int exponent;
> int64_t den;
> if (isnan(d))
> return (AVRational) { 0,0 };
> if (fabs(d) > INT_MAX + 3LL)
> return (AVRational) { d < 0 ? -1 : 1, 0 };
> -exponent = FFMAX( (int)(log(fabs(d) + 1e-20)/LOG2), 0);
> +frexp(d, &exponent);
> +exponent = FFMAX(exponent-1, 0);
> den = 1LL << (61 - exponent);
> // (int64_t)rint() and llrint() do not work with gcc on ia64 and sparc64
> av_reduce(&a.num, &a.den, floor(d * den + 0.5), den, max);
> --
> 2.6.2
>
Just a note, so that this is not buried: wm4's comment (which I still
disagree with) does not apply to this patch.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 01/11] ffmpeg: use log10 instead of log()/log(10)
On Thu, Oct 29, 2015 at 5:43 AM, wm4 wrote: > On Thu, 29 Oct 2015 00:19:59 -0400 > Ganesh Ajjanagadde wrote: > >> This is more concise and conveys the intent better. >> Furthermore, it is likely more precise as well due to lack of floating >> point division. >> >> Signed-off-by: Ganesh Ajjanagadde >> --- > > These patches are all pretty similar. And likely tedious to check for > correctness. So low gain, while some potential for regressions. In the time it took you to write that comment, you could have easily reviewed a couple. log10 is already being used in the codebase, why not make it consistent and also make it more precise? There is a good reason why libc has a log10 function. Maybe you don't care about such things, but I do: it is (roughly) analogous to using double instead of float for filters/resampling etc - noise floor should not be increased unless there is a clear benefit. Here there is none from using log as opposed to log10. > > NACK from me. > ___ > 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 4/8] tools/crypto_bench: add AES-192 and AES-256
Le septidi 7 brumaire, an CCXXIV, Rodger Combs a écrit : > --- > tools/crypto_bench.c | 82 > > 1 file changed, 82 insertions(+) Just a suggestion: maybe use macros to factor the code between the three key sizes. That would probably reduce the next patch about the libcrypto faster API. But since it was already approved, do not feel compelled. Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 7/8] tools/crypto_bench: add support for multiple lavu versions by cpuflag
Le septidi 7 brumaire, an CCXXIV, Rodger Combs a écrit :
> ---
> tools/crypto_bench.c | 50 ++
> 1 file changed, 42 insertions(+), 8 deletions(-)
>
> diff --git a/tools/crypto_bench.c b/tools/crypto_bench.c
> index 8a468ba..b513c55 100644
> --- a/tools/crypto_bench.c
> +++ b/tools/crypto_bench.c
> @@ -32,6 +32,7 @@
> #include "libavutil/crc.h"
> #include "libavutil/intreadwrite.h"
> #include "libavutil/timer.h"
> +#include "libavutil/cpu.h"
>
> #ifndef AV_READ_TIME
> #define AV_READ_TIME(x) 0
> @@ -65,6 +66,7 @@ struct hash_impl {
> const char *name;
> void (*run)(uint8_t *output, const uint8_t *input, unsigned size);
> const char *output;
> +int cpu_versions;
> };
>
> /***
> @@ -667,6 +669,16 @@ static unsigned crc32(const uint8_t *data, unsigned size)
> return av_crc(av_crc_get_table(AV_CRC_32_IEEE), 0, data, size);
> }
>
> +static const struct {
> +int flag;
> +const char *name;
> +} cpu_flag_tab[] = {
> +#if ARCH_X86
> +{ AV_CPU_FLAG_AESNI, "aesni" },
> +#endif
> +{ 0 }
> +};
> +
> static void run_implementation(const uint8_t *input, uint8_t *output,
> struct hash_impl *impl, unsigned size)
> {
> @@ -677,9 +689,31 @@ static void run_implementation(const uint8_t *input,
> uint8_t *output,
> double mtime, ttime = 0, ttime2 = 0, stime;
> uint8_t outref[MAX_OUTPUT_SIZE];
>
> +if (!strcmp(impl->lib, "lavu")) {
> +char lib_name[32];
> +struct hash_impl impl2 = *impl;
> +int real_flags = av_get_cpu_flags();
> +int current_flags = real_flags;
> +impl2.cpu_versions = 0;
> +impl2.lib = lib_name;
> +for (i = 0; cpu_flag_tab[i].flag; i++) {
> +if (cpu_flag_tab[i].flag & impl->cpu_versions & real_flags) {
> +snprintf(lib_name, sizeof(lib_name), "lavu_%s",
> cpu_flag_tab[i].name);
> +run_implementation(input, output, &impl2, size);
> +current_flags &= ~cpu_flag_tab[i].flag;
> +av_force_cpu_flags(current_flags);
> +}
> +}
> +impl2.lib = "lavu_c";
> +run_implementation(input, output, &impl2, size);
> +av_force_cpu_flags(real_flags);
> +return;
> +}
> +
I am still not very happy with the complex logic of the code, but I have no
good suggestion to make it simpler.
> if (enabled_libs && !av_stristr(enabled_libs, impl->lib) ||
> enabled_algos && !av_stristr(enabled_algos, impl->name))
> return;
> +
Stray.
> if (!sscanf(impl->output, "crc:%x", &outcrc)) {
> outlen = strlen(impl->output) / 2;
> for (i = 0; i < outlen; i++) {
> @@ -718,8 +752,8 @@ static void run_implementation(const uint8_t *input,
> uint8_t *output,
> fflush(stdout);
> }
>
> -#define IMPL_USE(lib, name, symbol, output) \
> -{ #lib, name, run_ ## lib ## _ ## symbol, output },
> +#define IMPL_USE(lib, name, symbol, output, ...) \
> +{ #lib, name, run_ ## lib ## _ ## symbol, output, __VA_ARGS__ },
> #define IMPL(lib, ...) IMPL_USE_ ## lib(lib, __VA_ARGS__)
> #define IMPL_ALL(...) \
> IMPL(lavu, __VA_ARGS__) \
> @@ -736,12 +770,12 @@ struct hash_impl implementations[] = {
> IMPL(lavu, "RIPEMD-128", ripemd128,
> "9ab8bfba2ddccc5d99c9d4cdfb844a5f")
> IMPL(tomcrypt, "RIPEMD-128", ripemd128,
> "9ab8bfba2ddccc5d99c9d4cdfb844a5f")
> IMPL_ALL("RIPEMD-160", ripemd160,
> "62a5321e4fc8784903bb43ab7752c75f8b25af00")
> -IMPL_ALL("AES-128-ECB",aes128,"crc:ff6bc888")
> -IMPL_ALL("AES-192-ECB",aes192,"crc:1022815b")
> -IMPL_ALL("AES-256-ECB",aes256,"crc:792e4e8a")
> -IMPL_ALL("AES-128-CBC",aes128cbc, "crc:0efebabe")
> -IMPL_ALL("AES-192-CBC",aes192cbc, "crc:ee2e34e8")
> -IMPL_ALL("AES-256-CBC",aes256cbc, "crc:0c9b875c")
> +IMPL_ALL("AES-128-ECB",aes128,"crc:ff6bc888", AV_CPU_FLAG_AESNI)
> +IMPL_ALL("AES-192-ECB",aes192,"crc:1022815b", AV_CPU_FLAG_AESNI)
> +IMPL_ALL("AES-256-ECB",aes256,"crc:792e4e8a", AV_CPU_FLAG_AESNI)
> +IMPL_ALL("AES-128-CBC",aes128cbc, "crc:0efebabe", AV_CPU_FLAG_AESNI)
> +IMPL_ALL("AES-192-CBC",aes192cbc, "crc:ee2e34e8", AV_CPU_FLAG_AESNI)
> +IMPL_ALL("AES-256-CBC",aes256cbc, "crc:0c9b875c", AV_CPU_FLAG_AESNI)
> IMPL_ALL("CAMELLIA", camellia, "crc:7abb59a7")
> IMPL_ALL("CAST-128", cast128, "crc:456aa584")
> IMPL_ALL("BLOWFISH", blowfish, "crc:33e8aa74")
Should be ok.
Regards,
--
Nicolas George
signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 6/8] tools/crypto_bench: switch to OpenSSL's faster AES API
Le septidi 7 brumaire, an CCXXIV, Rodger Combs a écrit : > --- > tools/crypto_bench.c | 85 > +--- > 1 file changed, 47 insertions(+), 38 deletions(-) I do not know enough of libcrypto API to comment on the actual code. No objection if it works. Could you include before/after numbers to the commit message to show how faster it is? Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 5/8] tools/crypto_bench: add AES-CBC modes
Le septidi 7 brumaire, an CCXXIV, Rodger Combs a écrit : > --- > tools/crypto_bench.c | 140 > +-- > 1 file changed, 137 insertions(+), 3 deletions(-) Should be ok. Not sure all the modes are really needed since the operation mode is quite orthogonal to the block cipher itself, but it does not matter much. Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] lavc/pngdec: honor skip_frame option
On Wed, Oct 28, 2015 at 5:35 PM, Michael Niedermayer wrote:
> On Wed, Oct 28, 2015 at 03:09:02PM +0100, Matthieu Bouron wrote:
> > On Wed, Oct 28, 2015 at 12:59:54PM +0100, Michael Niedermayer wrote:
> > > On Mon, Oct 19, 2015 at 10:28:18AM +0200, Matthieu Bouron wrote:
> > > > On Sat, Oct 17, 2015 at 10:34:22PM +0200, Matthieu Bouron wrote:
> > > > > From: Matthieu Bouron
> > > > >
> > > > > ---
> > > > > libavcodec/pngdec.c | 34 ++
> > > > > 1 file changed, 34 insertions(+)
> > > > >
> > > > > diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
> > > > > index 4cfdc58..5b2befe 100644
> > > > > --- a/libavcodec/pngdec.c
> > > > > +++ b/libavcodec/pngdec.c
> > > > > @@ -1087,6 +1087,13 @@ static int
> decode_frame_common(AVCodecContext *avctx, PNGDecContext *s,
> > > > > for (;;) {
> > > > > length = bytestream2_get_bytes_left(&s->gb);
> > > > > if (length <= 0) {
> > > > > +
> > > > > +if (avctx->codec_id == AV_CODEC_ID_PNG &&
> > > > > +avctx->skip_frame == AVDISCARD_ALL) {
> > > > > +av_frame_set_metadata(p, metadata);
> > > > > +return 0;
> > > > > +}
> > > > > +
> > > > > if (CONFIG_APNG_DECODER && avctx->codec_id ==
> AV_CODEC_ID_APNG && length == 0) {
> > > > > if (!(s->state & PNG_IDAT))
> > > > > return 0;
> > > > > @@ -1114,6 +1121,22 @@ static int
> decode_frame_common(AVCodecContext *avctx, PNGDecContext *s,
> > > > > ((tag >> 8) & 0xff),
> > > > > ((tag >> 16) & 0xff),
> > > > > ((tag >> 24) & 0xff), length);
> > > > > +
> > > > > +if (avctx->codec_id == AV_CODEC_ID_PNG &&
> > > > > +avctx->skip_frame == AVDISCARD_ALL) {
> > > > > +int i, found = 0;
> > > > > +static const int tags[] = { MKTAG('I', 'H', 'D',
> 'R'), MKTAG('t', 'E', 'X', 't'), MKTAG('I', 'D', 'A', 'T') };
> > > > > +for (i = 0; i < FF_ARRAY_ELEMS(tags); i++) {
> > > > > +if (tag == tags[i]) {
> > > > > +found = 1;
> > > > > +break;
> > > > > +}
> > > > > +}
> > > > > +if (!found) {
> > > > > +goto skip_tag;
> > > > > +}
> > > > > +}
> > > > > +
> > > > > switch (tag) {
> > > > > case MKTAG('I', 'H', 'D', 'R'):
> > > > > if ((ret = decode_ihdr_chunk(avctx, s, length)) < 0)
> > > > > @@ -1181,6 +1204,11 @@ skip_tag:
> > > > > }
> > > > > }
> > > > > exit_loop:
> > > > > +if (avctx->codec_id == AV_CODEC_ID_PNG &&
> > > > > +avctx->skip_frame == AVDISCARD_ALL) {
> > > > > +av_frame_set_metadata(p, metadata);
> > > > > +return 0;
> > > > > +}
> > > > >
> > > > > if (s->bits_per_pixel <= 4)
> > > > > handle_small_bpp(s, p);
> > > > > @@ -1278,6 +1306,12 @@ static int decode_frame_png(AVCodecContext
> *avctx,
> > > > > if ((ret = decode_frame_common(avctx, s, p, avpkt)) < 0)
> > > > > goto the_end;
> > > > >
> > > > > +if (avctx->skip_frame == AVDISCARD_ALL) {
> > > > > +*got_frame = 0;
> > > > > +ret = bytestream2_tell(&s->gb);
> > > > > +goto the_end;
> > > > > +}
> > > > > +
> > > > > if ((ret = av_frame_ref(data, s->picture.f)) < 0)
> > > > > return ret;
> > > > >
> > > > > --
> > > > > 2.6.1
> > > > >
> > > >
> > > > Patch updated, the SAR is not discarded anymore.
> > > >
> > > > Matthieu
> > >
> > > > pngdec.c | 39 +++
> > > > 1 file changed, 39 insertions(+)
> > > > eddfb042e2653025b37f9deacc0357ac3e5e5148
> 0001-lavc-pngdec-honor-skip_frame-option.patch
> > > > From ca6362457eb27e623eadcadb0e33a84ea461180f Mon Sep 17 00:00:00
> 2001
> > > > From: Matthieu Bouron
> > > > Date: Fri, 9 Oct 2015 15:14:11 +0200
> > > > Subject: [PATCH 1/3] lavc/pngdec: honor skip_frame option
> > > >
> > > > ---
> > > > libavcodec/pngdec.c | 39 +++
> > > > 1 file changed, 39 insertions(+)
> > > >
> > > > diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
> > > > index 4cfdc58..cb99dbe 100644
> > > > --- a/libavcodec/pngdec.c
> > > > +++ b/libavcodec/pngdec.c
> > > > @@ -1087,6 +1087,13 @@ static int decode_frame_common(AVCodecContext
> *avctx, PNGDecContext *s,
> > > > for (;;) {
> > > > length = bytestream2_get_bytes_left(&s->gb);
> > > > if (length <= 0) {
> > > > +
> > > > +if (avctx->codec_id == AV_CODEC_ID_PNG &&
> > > > +avctx->skip_frame == AVDISCARD_ALL) {
> > > > +av_frame_set_metadata(p, metadata);
> > > > +return 0;
> > > > +}
> > > > +
> > > > if (CONFIG_APNG_DECODER && avctx->codec_id ==
> AV_CODEC_ID_APNG && length == 0) {
> > > > if (!(s->state & PNG_IDAT))
> > >
Re: [FFmpeg-devel] [PATCH 2/3] lavc/mjpegdec: honor skip_frame option
On Wed, Oct 28, 2015 at 5:36 PM, Michael Niedermayer wrote:
> On Wed, Oct 28, 2015 at 03:10:00PM +0100, Matthieu Bouron wrote:
> > On Wed, Oct 28, 2015 at 12:58:53PM +0100, Michael Niedermayer wrote:
> > > On Wed, Oct 28, 2015 at 02:57:14AM +0100, Michael Niedermayer wrote:
> > > > On Tue, Oct 27, 2015 at 11:15:29PM +0100, Matthieu Bouron wrote:
> > > > > On Sun, Oct 18, 2015 at 11:06:50AM +0200, Matthieu Bouron wrote:
> > > > > [...]
> > > > > >
> > > > > > Patch updated, the markers are now properly skipped (which also
> fixes a
> > > > > > crash).
> > > > >
> > > > > Patch updated. It fixes an issue with mjpeg streams (and in
> particular with
> > > > > the fate sample ffmpeg-issue-897.avi) due to the EOI marker not
> handled when
> > > > > skip_frame is set to AVDISCARD_ALL.
> > > > >
> > > > > Matthieu
> > > >
> > > > > mjpegdec.c | 26 ++
> > > > > 1 file changed, 26 insertions(+)
> > > > > 36d41f1bded2f864394843c6a49d8cc24933688c
> 0002-lavc-mjpegdec-honor-skip_frame-option.patch
> > > > > From 7325810d812c4182cd42946687a1f4abc04999d1 Mon Sep 17 00:00:00
> 2001
> > > > > From: Matthieu Bouron
> > > > > Date: Fri, 9 Oct 2015 15:15:15 +0200
> > > > > Subject: [PATCH 2/3] lavc/mjpegdec: honor skip_frame option
> > > > >
> > > > > ---
> > > > > libavcodec/mjpegdec.c | 26 ++
> > > > > 1 file changed, 26 insertions(+)
> > > > >
> > > > > diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
> > > > > index 1a86b7b..b4ff83c 100644
> > > > > --- a/libavcodec/mjpegdec.c
> > > > > +++ b/libavcodec/mjpegdec.c
> > > > > @@ -2038,6 +2038,22 @@ int ff_mjpeg_decode_frame(AVCodecContext
> *avctx, void *data, int *got_frame,
> > > > > return AVERROR(ENOSYS);
> > > > > }
> > > > >
> > > > > +if (avctx->skip_frame == AVDISCARD_ALL) {
> > > > > +int i, found = 0;
> > > > > +static const int start_codes[] = { SOF0,
> > > > > +SOF1, SOF2, SOF3, SOF48, SOI, EOI };
> > > > > +
> > > > > +for (i = 0; i < FF_ARRAY_ELEMS(start_codes); i++) {
> > > > > +if (start_code == start_codes[i]) {
> > > > > +found = 1;
> > > > > +break;
> > > > > +}
> > > > > +}
> > > > > +if (!found) {
> > > > > +goto skip;
> > > > > +}
> > >
> > > i think this would be simpler and shorter if implemented as a
> > > switch(start_code)/case
> > >
> >
> > Patch updated using a switch/case to implement the skip logic.
> >
> > [...]
>
> > mjpegdec.c | 25 +
> > 1 file changed, 25 insertions(+)
> > 2db19e21ec69adc30ec5a3fdf03bd0c60257cb65
> 0002-lavc-mjpegdec-honor-skip_frame-option.patch
> > From cb09375d6cf5051ce51e43a2787f0242bd6450bc Mon Sep 17 00:00:00 2001
> > From: Matthieu Bouron
> > Date: Fri, 9 Oct 2015 15:15:15 +0200
> > Subject: [PATCH 2/3] lavc/mjpegdec: honor skip_frame option
>
> LGTM
>
Pushed.
Thanks
[...]
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] vp9_parser: don't overwrite cached timestamps with nopts.
Hi,
On Thu, Oct 29, 2015 at 12:45 AM, James Almer wrote:
> On 10/28/2015 11:16 PM, Ronald S. Bultje wrote:
> > Hi,
> >
> > On Wed, Oct 28, 2015 at 5:51 PM, wm4 wrote:
> >
> >> On Wed, 28 Oct 2015 16:05:49 -0400
> >> "Ronald S. Bultje" wrote:
> >>
> >>> Hi,
> >>>
> >>> On Wed, Oct 28, 2015 at 3:44 PM, wm4 wrote:
> >>>
> On Wed, 28 Oct 2015 12:21:05 -0400
> "Ronald S. Bultje" wrote:
>
> > ---
> > libavcodec/vp9_parser.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/vp9_parser.c b/libavcodec/vp9_parser.c
> > index 0437097..6713850 100644
> > --- a/libavcodec/vp9_parser.c
> > +++ b/libavcodec/vp9_parser.c
> > @@ -64,7 +64,7 @@ static int parse_frame(AVCodecParserContext *ctx,
> const uint8_t *buf, int size)
> > if (ctx->pts == AV_NOPTS_VALUE)
> > ctx->pts = s->pts;
> > s->pts = AV_NOPTS_VALUE;
> > -} else {
> > +} else if (ctx->pts != AV_NOPTS_VALUE) {
> > s->pts = ctx->pts;
> > ctx->pts = AV_NOPTS_VALUE;
> > }
>
> I find this a bit questionable. What is it needed for? Wouldn't it
> repeat the previous timestamp if new packets have none?
> >>>
> >>>
> >>> I don't think so. Let's first go through the problem that I'm seeing,
> >> maybe
> >>> you see alternate solutions. Then I'll explain (very end) why I don't
> >> think
> >>> this happens.
> >>>
> >>> So, we're assuming here (this is generally true) that all input to the
> >>> parser has timestamps (coming from ivf/webm), and that some frames are
> >>> "superframes" which have one invisible (alt-ref) frame (only a
> reference,
> >>> not an actual frame that's ever displayed) packed with the following
> >>> visible frame. So each packet in ivf/webm leads to one visible output
> >>> frame, but in some cases this requires decoding of multiple (invisible)
> >>> references. For frame threading purposes, we split before we send it to
> >> the
> >>> decoder.
> >>>
> >>> So what you get is:
> >>> - ivf/webm file has packet of size a with timestamp b, calls parser
> >>> - parser notices that packet is superframe with 2 frames in it
> >>> - we output the first (invisible) frame with no timestamp, and cache
> the
> >>> timestamp of the input packet
> >>> - utils.c code calls parser again with the same input data (we told it
> we
> >>> didn't consume any), but the (input) timestamp is now set to nopts
> >>> - we output the second (visible) frame with the cached timestamp on the
> >>> packet
> >>>
> >>> This generally makes sense, the webm/ivf indeed assume that the
> timestamp
> >>> only is valid for the visible frame. Invisible frames have no timestamp
> >>> associated with them since they're never displayed.
> >>>
> >>> So, the above code has the issue: what if there's 2 invisible frames
> >> packed
> >>> in a superframe (followed by the visible frame)? Right now, this
> happens:
> >>> - ivf/webm file has packet of size a with timestamp b, calls parser
> >>> - parser notices that packet is superframe with 3 frames in it
> >>> - we output the first (invisible) frame with no timestamp, and cache
> the
> >>> timestamp of the input packet
> >>> - utils.c code calls parser again with the same input data (we told it
> we
> >>> didn't consume any), but the (input) timestamp is now set to nopts
> >>> - we output the second (invisible) frame with no timestamp, and cache
> the
> >>> timestamp of the input packet (which is now set to nopts)
> >>> - utils.c code calls parser again with the same input data (we told it
> we
> >>> didn't consume any), but the (input) timestamp is now set to nopts
> >>> - we output the third (visible) frame with the cached timestamp on the
> >>> packet, which was nopts
> >>>
> >>> The last 3 are wrong; we should only cache timestamps if there is any
> to
> >> be
> >>> cached, we should not override the cached timestamp with a new nopts
> >> value,
> >>> at least not in this particular case.
> >>>
> >>> --
> >>> very end
> >>> --
> >>>
> >>> Ok, so what about your point: can we output the same timestamp twice? I
> >>> don't think so, because as soon as we output the cached timestamp, we
> >> reset
> >>> the value of the cached timestamp to nopts, so if we were to reuse the
> >>> cached timestamp, it would be nopts and the output data would have no
> >>> timestamp.
> >>>
> >>> (Hope that wasn't too detailed.)
> >>
> >> Thanks for the explanations. I didn't know there could be more than 1
> >> super frame, and I see how the new logic works and doesn't duplicate
> >> timestamps.
> >>
> >> Patch looks good with me then.
> >
> >
> > TY, pushed.
> >
> > Ronald
>
> Did you forget to update the ref files for fate-vp9-10-show-existing-frame
> and
> fate-vp9-16-intra-only? Because they are failing in a lot of fate clients.
Oops, I will work on it.
Ronald
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.
Re: [FFmpeg-devel] [PATCH 01/11] ffmpeg: use log10 instead of log()/log(10)
On Thu, 29 Oct 2015 00:19:59 -0400 Ganesh Ajjanagadde wrote: > This is more concise and conveys the intent better. > Furthermore, it is likely more precise as well due to lack of floating > point division. > > Signed-off-by: Ganesh Ajjanagadde > --- These patches are all pretty similar. And likely tedious to check for correctness. So low gain, while some potential for regressions. NACK from me. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] WIP: lavf/utils: try to avoid decoding a frame to get the codec parameters
On Thu, 29 Oct 2015 08:34:42 +0100
Clément Bœsch wrote:
> On Wed, Oct 28, 2015 at 06:34:06PM +0100, Hendrik Leppkes wrote:
> > On Sat, Oct 17, 2015 at 10:34 PM, Matthieu Bouron
> > wrote:
> > > From: Matthieu Bouron
> > >
> > > Avoid decoding twice images such as jpeg and png, once in the
> > > avformat_find_stream_info and once when the actual decode is made.
> > >
> > > The decoder must honor the skip_frame option in order to skip
> > > decoding. For now the AVDISCARD_ALL flag is only set for the mjpeg and
> > > png decoders.
> > > ---
> > > libavformat/utils.c | 15 +++
> > > 1 file changed, 15 insertions(+)
> > >
> > > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > > index 689473e..67dfffc 100644
> > > --- a/libavformat/utils.c
> > > +++ b/libavformat/utils.c
> > > @@ -2676,11 +2676,16 @@ static int has_codec_parameters(AVStream *st,
> > > const char **errmsg_ptr)
> > > static int try_decode_frame(AVFormatContext *s, AVStream *st, AVPacket
> > > *avpkt,
> > > AVDictionary **options)
> > > {
> > > +int i;
> > > const AVCodec *codec;
> > > int got_picture = 1, ret = 0;
> > > AVFrame *frame = av_frame_alloc();
> > > AVSubtitle subtitle;
> > > AVPacket pkt = *avpkt;
> > > +int skip_frame;
> > > +static const enum AVCodecID no_decode_codecs[] = {
> > > +AV_CODEC_ID_MJPEG, AV_CODEC_ID_PNG,
> > > +};
> >
> > Hardcoded lists of codecs in random places are never a good thing to have.
> > If this is a feature we agree to have, its codecs should just get an
> > internal capability that tells this code if it can parse all params
> > without decoding.
> >
>
> This list is supposed to be temporary (yes I know) until all other
> decoders that currently support AVDISCARD_ALL set the information field
> required for probing.
I do not think this is the right direction. We should go away from
mutating AVCodecContext fields, towards returning all per-frame info in
AVFrame. We should also go away from decoding in libavformat.
Before this becomes final, maybe you could check whether this could be
done in a codec parser instead? Some parsers do set the pixfmt, e.g.
h264. Yes, this suggestion is a bit late. I don't actually mind this
hack being applied in this form; at least it makes it explicit that
it's a hack for the image formats, and can be easily fixed at a later
point.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil/opt: add av_warn_unused_result
On Thu, 29 Oct 2015 09:17:16 +0100 Hendrik Leppkes wrote: > On Thu, Oct 29, 2015 at 3:51 AM, Ganesh Ajjanagadde > wrote: > > On Thu, Oct 15, 2015 at 6:12 PM, Ganesh Ajjanagadde > > wrote: > >> Some warnings do get triggered that will need to be fixed. > >> > >> Signed-off-by: Ganesh Ajjanagadde > >> --- > >> libavutil/opt.h | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/libavutil/opt.h b/libavutil/opt.h > >> index 753434d..5abe963 100644 > >> --- a/libavutil/opt.h > >> +++ b/libavutil/opt.h > >> @@ -414,6 +414,7 @@ void av_opt_set_defaults2(void *s, int mask, int > >> flags); > >> * the error code issued by av_opt_set() if a key/value pair > >> * cannot be set > >> */ > >> +av_warn_unused_result > >> int av_set_options_string(void *ctx, const char *opts, > >>const char *key_val_sep, const char *pairs_sep); > >> > >> @@ -444,6 +445,7 @@ int av_set_options_string(void *ctx, const char *opts, > >> * Separators must use characters distinct from option names and from each > >> * other. > >> */ > >> +av_warn_unused_result > >> int av_opt_set_from_string(void *ctx, const char *opts, > >> const char *const *shorthand, > >> const char *key_val_sep, const char > >> *pairs_sep); > >> -- > >> 2.6.1 > >> > > > > ping > > I would argue that failure from these functions is hardly critical, > ie. it can fail if an option is not found, and enforcing checks here > seems a bit too aggressive to me. +1 You might often set options on e.g. a codec generically, without caring much whether it fails or not. This is because different codecs have different options, and those which lack a certain option will work without setting it. (Just as an example.) ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] Idea for a better filter that reduces noise
On 10/29/15, P. van Gaans wrote: > You all know the CSI episodes where they read a license plate by > "enhancing" some super-grainy security footage. Nonsense, right? Well, > maybe not.. If the car was parked. And it seems what I found doesn't > exist yet. (but perhaps I overlooked it) > > If you quickly want to know what I'm on about, take a look at these images: > > http://members.ziggo.nl/sinaasappel/images/1_original.jpg (original) > http://members.ziggo.nl/sinaasappel/images/2_40framewind.jpg (40f WIND) > http://members.ziggo.nl/sinaasappel/images/3_wind_hqdn3d.jpg (comparison > with hqdn3d and pp=tn) > > All have limited jpeg compression, but I can deliver PNG files and an > XCF to experiment for yourself if desired. > > So what is WIND? It's what you see if you forget/fail to do motion > detection. (like I did in the images) Also, Wind Is Not a Denoiser. ;-) > It's a way to increase the exposure time of the camera used to shoot a > movie after it's been shot. For the images, I took a 2-second somewhat > grainy clip of a building with nearly no motion. I output the frames to > PNG and load them as layers in The GIMP. I set the opacity for the > bottom layer to 100. The layer above that 50. (100/2) Above that 33.3. > (100/3) 25, 20, 16.7 and so on. Every image with noise is "wrong", some > too dark and some too light. On average, they are spot-on. The > advantage: improved quality and better compression. > > To make this into a proper useable filter, it would need to do this: > > 1. Deshake/stabilize the image. > 2. Divide image in blocks. (e.g. 8x8 pixels) > 3. Figure out it the average color of an 8x8 block is changing during > the next X frames. If not, it's probably not moving and you can average > the values. If it is or is about to, it should be averaged over fewer > frames or not at all. Any area that is about to move will gradually pick > up noise so it doesn't look too unnatural. > 4. Reshake the image. > > Will I do this myself? Maybe, but don't hold your breath. I'm just > sharing this in case somebody finds it interesting and to make sure > nobody can patent it. (assuming it hasn't been done already) See atadenoise. > > Best regards, > > P. van Gaans > ___ > 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
[FFmpeg-devel] [PATCH] avutil/file_open: avoid file handle inheritance on Windows
Attached patch fixes file lock issues in my Windows application when a child process is started with handle inheritance enabled (standard input/output redirection) while a FFmpeg transcoding is running in the parent process. BTW: It would be great if the patch could also be applied to the 2.7/2.8 release branches. Regards, Tobias >From b54a2177aaf0aef290c0741a63327ca678b4e7d6 Mon Sep 17 00:00:00 2001 From: Tobias Rapp Date: Thu, 29 Oct 2015 09:11:37 +0100 Subject: [PATCH] avutil/file_open: avoid file handle inheritance on Windows Avoids inheritance of file handles on Windows systems similar to the O_CLOEXEC/FD_CLOEXEC flag on Linux. Fixes file lock issues in my Windows application when a child process is started with handle inheritance enabled (standard input/output redirection) while a FFmpeg transcoding is running in the parent process. Signed-off-by: Tobias Rapp --- libavutil/file_open.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/libavutil/file_open.c b/libavutil/file_open.c index 3f9a67c..9e76127 100644 --- a/libavutil/file_open.c +++ b/libavutil/file_open.c @@ -77,6 +77,9 @@ int avpriv_open(const char *filename, int flags, ...) #ifdef O_CLOEXEC flags |= O_CLOEXEC; #endif +#ifdef O_NOINHERIT +flags |= O_NOINHERIT; +#endif fd = open(filename, flags, mode); #if HAVE_FCNTL -- 1.9.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] WIP: lavf/utils: try to avoid decoding a frame to get the codec parameters
On Thu, Oct 29, 2015 at 8:34 AM, Clément Bœsch wrote:
> On Wed, Oct 28, 2015 at 06:34:06PM +0100, Hendrik Leppkes wrote:
>> On Sat, Oct 17, 2015 at 10:34 PM, Matthieu Bouron
>> wrote:
>> > From: Matthieu Bouron
>> >
>> > Avoid decoding twice images such as jpeg and png, once in the
>> > avformat_find_stream_info and once when the actual decode is made.
>> >
>> > The decoder must honor the skip_frame option in order to skip
>> > decoding. For now the AVDISCARD_ALL flag is only set for the mjpeg and
>> > png decoders.
>> > ---
>> > libavformat/utils.c | 15 +++
>> > 1 file changed, 15 insertions(+)
>> >
>> > diff --git a/libavformat/utils.c b/libavformat/utils.c
>> > index 689473e..67dfffc 100644
>> > --- a/libavformat/utils.c
>> > +++ b/libavformat/utils.c
>> > @@ -2676,11 +2676,16 @@ static int has_codec_parameters(AVStream *st,
>> > const char **errmsg_ptr)
>> > static int try_decode_frame(AVFormatContext *s, AVStream *st, AVPacket
>> > *avpkt,
>> > AVDictionary **options)
>> > {
>> > +int i;
>> > const AVCodec *codec;
>> > int got_picture = 1, ret = 0;
>> > AVFrame *frame = av_frame_alloc();
>> > AVSubtitle subtitle;
>> > AVPacket pkt = *avpkt;
>> > +int skip_frame;
>> > +static const enum AVCodecID no_decode_codecs[] = {
>> > +AV_CODEC_ID_MJPEG, AV_CODEC_ID_PNG,
>> > +};
>>
>> Hardcoded lists of codecs in random places are never a good thing to have.
>> If this is a feature we agree to have, its codecs should just get an
>> internal capability that tells this code if it can parse all params
>> without decoding.
>>
>
> This list is supposed to be temporary (yes I know) until all other
> decoders that currently support AVDISCARD_ALL set the information field
> required for probing.
>
We all know what happens to temporary solutions, don't we. :)
Rather have a temporary internal capability, its not part of the
API/ABI or anything, but avoids such an ugly list inside generic code.
- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil/opt: add av_warn_unused_result
On Thu, Oct 29, 2015 at 3:51 AM, Ganesh Ajjanagadde wrote: > On Thu, Oct 15, 2015 at 6:12 PM, Ganesh Ajjanagadde > wrote: >> Some warnings do get triggered that will need to be fixed. >> >> Signed-off-by: Ganesh Ajjanagadde >> --- >> libavutil/opt.h | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/libavutil/opt.h b/libavutil/opt.h >> index 753434d..5abe963 100644 >> --- a/libavutil/opt.h >> +++ b/libavutil/opt.h >> @@ -414,6 +414,7 @@ void av_opt_set_defaults2(void *s, int mask, int flags); >> * the error code issued by av_opt_set() if a key/value pair >> * cannot be set >> */ >> +av_warn_unused_result >> int av_set_options_string(void *ctx, const char *opts, >>const char *key_val_sep, const char *pairs_sep); >> >> @@ -444,6 +445,7 @@ int av_set_options_string(void *ctx, const char *opts, >> * Separators must use characters distinct from option names and from each >> * other. >> */ >> +av_warn_unused_result >> int av_opt_set_from_string(void *ctx, const char *opts, >> const char *const *shorthand, >> const char *key_val_sep, const char *pairs_sep); >> -- >> 2.6.1 >> > > ping I would argue that failure from these functions is hardly critical, ie. it can fail if an option is not found, and enforcing checks here seems a bit too aggressive to me. - Hendrik ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] WIP: lavf/utils: try to avoid decoding a frame to get the codec parameters
On Thu, Oct 29, 2015 at 08:34:42AM +0100, Clément Bœsch wrote:
> On Wed, Oct 28, 2015 at 06:34:06PM +0100, Hendrik Leppkes wrote:
> > On Sat, Oct 17, 2015 at 10:34 PM, Matthieu Bouron
> > wrote:
> > > From: Matthieu Bouron
> > >
> > > Avoid decoding twice images such as jpeg and png, once in the
> > > avformat_find_stream_info and once when the actual decode is made.
> > >
> > > The decoder must honor the skip_frame option in order to skip
> > > decoding. For now the AVDISCARD_ALL flag is only set for the mjpeg and
> > > png decoders.
> > > ---
> > > libavformat/utils.c | 15 +++
> > > 1 file changed, 15 insertions(+)
> > >
> > > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > > index 689473e..67dfffc 100644
> > > --- a/libavformat/utils.c
> > > +++ b/libavformat/utils.c
> > > @@ -2676,11 +2676,16 @@ static int has_codec_parameters(AVStream *st,
> > > const char **errmsg_ptr)
> > > static int try_decode_frame(AVFormatContext *s, AVStream *st, AVPacket
> > > *avpkt,
> > > AVDictionary **options)
> > > {
> > > +int i;
> > > const AVCodec *codec;
> > > int got_picture = 1, ret = 0;
> > > AVFrame *frame = av_frame_alloc();
> > > AVSubtitle subtitle;
> > > AVPacket pkt = *avpkt;
> > > +int skip_frame;
> > > +static const enum AVCodecID no_decode_codecs[] = {
> > > +AV_CODEC_ID_MJPEG, AV_CODEC_ID_PNG,
> > > +};
> >
> > Hardcoded lists of codecs in random places are never a good thing to have.
> > If this is a feature we agree to have, its codecs should just get an
> > internal capability that tells this code if it can parse all params
> > without decoding.
> >
>
> This list is supposed to be temporary (yes I know) until all other
> decoders that currently support AVDISCARD_ALL set the information field
> required for probing.
>
> Not full frames decoding at probing but just filling the information will
> also be useful for these codecs (I think there are about 20?).
> Unfortunately, the changes are sensible so it's probably better to do it
> progressively one by one, updating this white list progressively. A
> blacklist system was also suggested earlier in the thread.
>
> Maybe a FIXME/XXX should be added above? (And maybe even with a compiler
> warning to not forget this?)
>
And forgot to say: if you choose to add a capability, it will have to be
dropped when the other decoders are updated to match the behaviour.
--
Clément B.
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] WIP: lavf/utils: try to avoid decoding a frame to get the codec parameters
On Wed, Oct 28, 2015 at 06:34:06PM +0100, Hendrik Leppkes wrote:
> On Sat, Oct 17, 2015 at 10:34 PM, Matthieu Bouron
> wrote:
> > From: Matthieu Bouron
> >
> > Avoid decoding twice images such as jpeg and png, once in the
> > avformat_find_stream_info and once when the actual decode is made.
> >
> > The decoder must honor the skip_frame option in order to skip
> > decoding. For now the AVDISCARD_ALL flag is only set for the mjpeg and
> > png decoders.
> > ---
> > libavformat/utils.c | 15 +++
> > 1 file changed, 15 insertions(+)
> >
> > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > index 689473e..67dfffc 100644
> > --- a/libavformat/utils.c
> > +++ b/libavformat/utils.c
> > @@ -2676,11 +2676,16 @@ static int has_codec_parameters(AVStream *st, const
> > char **errmsg_ptr)
> > static int try_decode_frame(AVFormatContext *s, AVStream *st, AVPacket
> > *avpkt,
> > AVDictionary **options)
> > {
> > +int i;
> > const AVCodec *codec;
> > int got_picture = 1, ret = 0;
> > AVFrame *frame = av_frame_alloc();
> > AVSubtitle subtitle;
> > AVPacket pkt = *avpkt;
> > +int skip_frame;
> > +static const enum AVCodecID no_decode_codecs[] = {
> > +AV_CODEC_ID_MJPEG, AV_CODEC_ID_PNG,
> > +};
>
> Hardcoded lists of codecs in random places are never a good thing to have.
> If this is a feature we agree to have, its codecs should just get an
> internal capability that tells this code if it can parse all params
> without decoding.
>
This list is supposed to be temporary (yes I know) until all other
decoders that currently support AVDISCARD_ALL set the information field
required for probing.
Not full frames decoding at probing but just filling the information will
also be useful for these codecs (I think there are about 20?).
Unfortunately, the changes are sensible so it's probably better to do it
progressively one by one, updating this white list progressively. A
blacklist system was also suggested earlier in the thread.
Maybe a FIXME/XXX should be added above? (And maybe even with a compiler
warning to not forget this?)
[...]
--
Clément B.
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
