Re: [FFmpeg-devel] avcodec/proresenc_aw : add support for prores 444
> > > > Speed (from prores 422 HQ file -> Prores 444) > > > > ./ffmpeg -i prores422hqfile.mov -pix_fmt yuv444p10 -c:v prores -an > > -profile:v 4 res_aw.mov -y > > ==> 25 fps > > > > ./ffmpeg -i prores422hqfile.mov -pix_fmt yuv444p10 -c:v prores_ks -an > > -profile:v 4 res_ks.mov -y > > ==> 10 fps > > This is unfair comparison. > What command line seems fair to you ? Even if, i set the same min/max quant (1, 6) and reducing the target bitrate (to have similar output size), the speed difference is very important. The resulting file is very close in quality (most of the picture is the same), on few part there is a very little difference in favour of one or another encoder depending of the content. > > > > 001 : Add support for Prores 444 (without alpha) > > 002 : change the frame flag in header part, to be the same than the > > official encoder in 422 mode > > (and update fate test) > > Whatever you are doing regarding improving prores_aw is bad idea. > > > I disagree (of course !). And i suppose you say that, because you have in mind to remove this encoder. After taking a look on both encoder, prores_aw have a faster quantif search, than prores_ks for me. Ks encoder sometime reduce too much the quality on some slice, where aw, is much simpler, and make less quality variation between slice. IMHO : Considering, the main use of prores codec (intermediate format in post-production) A simple/fast encoder (Prores Aw) is more appropriate in most of the case (reason why i mainly use this one, when i can). The main disadvantage (for now), for aw encoder, is lack of some user option (to adapt speed/size/bitrate constraint (in other word, min_quant, max_quant, target_mb_bitrate), and the support of other kind of prores (444 with alpha, and interlace content) But probably not so much work to do to add it (considering, the work already done in ks encoder). Martin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] avcodec/proresenc_aw improvements
> > > 001 : use scantable in prores_data instead of a duplicate one. > > This could negativly affect the performance of the changed inner loop > have you checked that the changed function does not become slower ? > > > No, i will make some tests. > > -*buf++ = 6; > > +*buf++ = pict->color_primaries; > > +*buf++ = pict->color_trc; > > +*buf++ = pict->colorspace; > > Has someone confirmed that all values our enum contains or will > contain can just be written with no check ? > > Mostly see prores with Rec709, Rec601. Need to take a look for Rec2020. Don't know if all value are authorized, doesn't use other colorspace, with prores. Do you think it's better to only authorize few colorspace ? Martin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] lavfi/motion_estimation: use pixelutils API for sad.
On Tue, 17 Jul 2018, myp...@gmail.com wrote: On Sun, Jul 15, 2018 at 1:03 AM Michael Niedermayer wrote: On Sat, Jul 14, 2018 at 12:04:46PM +0200, Marton Balint wrote: > > > On Sat, 14 Jul 2018, Michael Niedermayer wrote: > > >On Fri, Jul 13, 2018 at 10:51:00AM +0200, Marton Balint wrote: > >> > >> > >>On Thu, 12 Jul 2018, myp...@gmail.com wrote: > >> > >>>On Thu, Jul 12, 2018 at 12:43 AM Marton Balint wrote: > > > > On Wed, 11 Jul 2018, Jun Zhao wrote: > > >use pixelutils API for sad in motion estimation. > > Does it make sense to improve this code? I thought a superior and faster > approach was a result of 2017 GSOC task: > > https://docs.google.com/document/d/1Hyh_rxP1KGsVkg7i7yU8Bcv92z0LIL4r-axpoKfvMFk/edit > > Maybe that code should be merged back, and any further optimalization > should be done based on that code, no? > > Thanks, > Marton > > >>>Hi, Marton: > >>> > >>>Yes, now I try to improve the minterpolate, and after use perf > >>>profiing the commands: > >>> > >>>./ffmpeg -i a.ts -filter_complex > >>>"minterpolate=mi_mode=mci:mc_mode=aobmc:vsbmc=1" -f null /dev/null > >>>I found the hotspot is: > >>>- get_sbad_ob > >>>- get_sbad > >>>- get_sad_ob > >>>- bilateral_obmc > >>>- set_frame_data > >>> > >>>So, as my plan, I will try to use sse2/avx2 > >>>Scatter/Gather, optimized > >>>sad function (use pixelutils API) > >>>in get_sbad_ob / get_sbad / get_sad_ob first, for set_frame_data > >>>case, maybe need to use Scatter/Gather SIMD instruction. > >> > >>That is great, all I am saying we should avoid diverging the two brances > >>(FFmpeg branch, and GSOC 2017 branch), and try to merge back GSOC2017 if it > >>can be done with reasonable amount of work before optimizing code, otherwise > >>the GSOC2017 branch will rot and we will lose the result of the GSOC task. > >> > >>> > >>>But if some guys have done some improve task in this case, I think > >>>based on the pre-existing work is the better way. > >> > >>Michael was the mentor, maybe he can chip in on what should be done here. > > > >talk with the author/student who wrote the code, not me :) > > Well, his not active here, yes but last i heared from him, he was interrested in continuing this project i think ive not heared much from him after that but i now see that there is a small commit in his repo from 2018 so he is not completely inactive. I think you should talk with him > and the question is if his work is ready for > mainline inclusion or not, and if he has done enough valuable work during > GSOC that its worth working on mainlining it. He certainly did valuable work. Looking now at the ML, it seems the more or less last thing on the ML was the RFC/Discussion thread about libmotion. In that everyone wanted to dictate the design, and all that was contradicting each other. If you want to work on unifying this entangled bikeshed ball of conflicting oppinions, that surely is very welcome. Important is that it ends in something that is practical and high quality. Personally i think the author should be given more authority in the design. But again, please talk with the author of this code I dont remember everything in as much detail about this ... also ive added him to the CC Thanks Now the minterpolate/libmotion auther didn't give a feedback or sugesstion, so I will update patch 1/2 (just add SSE2/AVX2 sad_32x32) with some perf data and hold on the patch 3 about minterpolate, any other comments? I checked the "libmotion" series, and it seems they are in debug/development state and the commits are not clean, so some heavy refactoring is needed before applying them anyway. Do what you prefer, snow codec based motion compenstaion is an additional algorithm to the existing code anyway as far as I see. Thanks, Marton ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 2/5] avcodec/dirac_dwt_template: Fix several integer overflows in horizontal_compose_daub97i()
Fixes: signed integer overflow: 2147483647 + 1 cannot be represented in type 'int' Fixes: 8926/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_DIRAC_fuzzer-6047609228623872 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer --- libavcodec/dirac_dwt_template.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libavcodec/dirac_dwt_template.c b/libavcodec/dirac_dwt_template.c index 2369c8d15b..5d55d932a1 100644 --- a/libavcodec/dirac_dwt_template.c +++ b/libavcodec/dirac_dwt_template.c @@ -190,15 +190,15 @@ static void RENAME(horizontal_compose_daub97i)(uint8_t *_b, uint8_t *_temp, int // second stage combined with interleave and shift b0 = b2 = COMPOSE_DAUB97iL0(temp[w2], temp[0], temp[w2]); -b[0] = (b0 + 1) >> 1; +b[0] = ~((~b0) >> 1); for (x = 1; x < w2; x++) { b2 = COMPOSE_DAUB97iL0(temp[x+w2-1], temp[x ], temp[x+w2]); b1 = COMPOSE_DAUB97iH0( b0, temp[x+w2-1], b2); -b[2*x-1] = (b1 + 1) >> 1; -b[2*x ] = (b2 + 1) >> 1; +b[2*x-1] = ~((~b1) >> 1); +b[2*x ] = ~((~b2) >> 1); b0 = b2; } -b[w-1] = (COMPOSE_DAUB97iH0(b2, temp[w-1], b2) + 1) >> 1; +b[w-1] = ~((~COMPOSE_DAUB97iH0(b2, temp[w-1], b2)) >> 1); } static void RENAME(vertical_compose_dirac53iH0)(uint8_t *_b0, uint8_t *_b1, uint8_t *_b2, -- 2.18.0 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 5/5] avcodec/diracdec: Check bytes count in else branch in decode_lowdelay() too
Fixes: signed integer overflow: 8 * 340018243 cannot be represented in type 'int' Fixes: 9441/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_DIRAC_fuzzer-5194665207791616 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer --- libavcodec/diracdec.c | 4 1 file changed, 4 insertions(+) diff --git a/libavcodec/diracdec.c b/libavcodec/diracdec.c index b27c743c58..9a417caec5 100644 --- a/libavcodec/diracdec.c +++ b/libavcodec/diracdec.c @@ -986,6 +986,10 @@ static int decode_lowdelay(DiracContext *s) for (slice_x = 0; bufsize > 0 && slice_x < s->num_x; slice_x++) { bytes = (slice_num+1) * (int64_t)s->lowdelay.bytes.num / s->lowdelay.bytes.den - slice_num* (int64_t)s->lowdelay.bytes.num / s->lowdelay.bytes.den; +if (bytes >= INT_MAX || bytes*8 > bufsize) { +av_log(s->avctx, AV_LOG_ERROR, "too many bytes\n"); +return AVERROR_INVALIDDATA; +} slices[slice_num].bytes = bytes; slices[slice_num].slice_x = slice_x; slices[slice_num].slice_y = slice_y; -- 2.18.0 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/5] avcodec/diracdec: Prevent integer overflow in intermediate in global_mv()
Fixes: signed integer overflow: -393471 * 5460 cannot be represented in type 'int' Fixes: 8890/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_DIRAC_fuzzer-6299775379963904 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer --- libavcodec/diracdec.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libavcodec/diracdec.c b/libavcodec/diracdec.c index 753adeff61..e4cdd7ee2c 100644 --- a/libavcodec/diracdec.c +++ b/libavcodec/diracdec.c @@ -1399,8 +1399,8 @@ static void global_mv(DiracContext *s, DiracBlock *block, int x, int y, int ref) int *c = s->globalmc[ref].perspective; int m = (1> (ez+ep); block->u.mv[ref][1] = (my + (1<<(ez+ep))) >> (ez+ep); -- 2.18.0 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 4/5] avcodec/diracdec: Check slice numbers for overflows in relation to picture dimensions
Fixes: signed integer overflow: 88 * 33685506 cannot be represented in type 'int' Fixes: 9433/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_DIRAC_fuzzer-5725943535501312 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer --- libavcodec/diracdec.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/libavcodec/diracdec.c b/libavcodec/diracdec.c index 4ef1b3ea9b..b27c743c58 100644 --- a/libavcodec/diracdec.c +++ b/libavcodec/diracdec.c @@ -1243,7 +1243,10 @@ static int dirac_unpack_idwt_params(DiracContext *s) else { s->num_x= get_interleaved_ue_golomb(gb); s->num_y= get_interleaved_ue_golomb(gb); -if (s->num_x * s->num_y == 0 || s->num_x * (uint64_t)s->num_y > INT_MAX) { +if (s->num_x * s->num_y == 0 || s->num_x * (uint64_t)s->num_y > INT_MAX || +s->num_x * (uint64_t)s->avctx->width > INT_MAX || +s->num_y * (uint64_t)s->avctx->height > INT_MAX +) { av_log(s->avctx,AV_LOG_ERROR,"Invalid numx/y\n"); s->num_x = s->num_y = 0; return AVERROR_INVALIDDATA; -- 2.18.0 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 3/5] avcodec/diracdec: Change frame_number to 64bit as its a 32bit from the bitstream and we also have a -1 special case
Fixes: signed integer overflow: 2147483647 + 1 cannot be represented in type 'int' Fixes: 9291/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_DIRAC_fuzzer-6324345860259840 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer --- libavcodec/diracdec.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libavcodec/diracdec.c b/libavcodec/diracdec.c index e4cdd7ee2c..4ef1b3ea9b 100644 --- a/libavcodec/diracdec.c +++ b/libavcodec/diracdec.c @@ -141,7 +141,7 @@ typedef struct DiracContext { GetBitContext gb; AVDiracSeqHeader seq; int seen_sequence_header; -int frame_number; /* number of the next frame to display */ +int64_t frame_number; /* number of the next frame to display */ Plane plane[3]; int chroma_x_shift; int chroma_y_shift; @@ -2310,7 +2310,7 @@ static int dirac_decode_frame(AVCodecContext *avctx, void *data, int *got_frame, } if (*got_frame) -s->frame_number = picture->display_picture_number + 1; +s->frame_number = picture->display_picture_number + 1LL; return buf_idx; } -- 2.18.0 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/5] avcodec/tscc: Move reading the side data palette before other checks
On Sun, Jul 15, 2018 at 03:13:39PM +0200, Michael Niedermayer wrote: > We do not want to loose the side data in case of errors > > Signed-off-by: Michael Niedermayer > --- > libavcodec/tscc.c | 23 ++- > 1 file changed, 14 insertions(+), 9 deletions(-) will apply [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Modern terrorism, a quick summary: Need oil, start war with country that has oil, kill hundread thousand in war. Let country fall into chaos, be surprised about raise of fundamantalists. Drop more bombs, kill more people, be surprised about them taking revenge and drop even more bombs and strip your own citizens of their rights and freedoms. to be continued signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 4/5] avcodec/tscc: Do not duplicate images
On Sun, Jul 15, 2018 at 03:13:40PM +0200, Michael Niedermayer wrote: > This improves speed > > Fixes: Timeout > Fixes: > 9010/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_TSCC_fuzzer-6042614817095680 > > Found-by: continuous fuzzing process > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer > --- > libavcodec/tscc.c | 6 +- > tests/ref/fate/tscc-15bit | 138 -- > tests/ref/fate/tscc-32bit | 121 - > 3 files changed, 4 insertions(+), 261 deletions(-) will apply with a bugfix [...] -- 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: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 5/5] avcodec/cdgraphics: Clear first frame only once
On Sun, Jul 15, 2018 at 03:13:41PM +0200, Michael Niedermayer wrote: > frame_number will not increase if nothing is output > > Fixes: Timeout > Fixes: > 9057/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CDGRAPHICS_fuzzer-4844661498707968 > > Found-by: continuous fuzzing process > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer > --- > libavcodec/cdgraphics.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) will apply [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Take away the freedom of one citizen and you will be jailed, take away the freedom of all citizens and you will be congratulated by your peers in Parliament. signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] avcodec/proresenc_aw improvements
On Sun, Jul 22, 2018 at 01:04:29PM +0200, Martin Vignali wrote: > > > > > 001 : use scantable in prores_data instead of a duplicate one. > > > > This could negativly affect the performance of the changed inner loop > > have you checked that the changed function does not become slower ? > > > > > > > No, i will make some tests. > > > > -*buf++ = 6; > > > +*buf++ = pict->color_primaries; > > > +*buf++ = pict->color_trc; > > > +*buf++ = pict->colorspace; > > > > Has someone confirmed that all values our enum contains or will > > contain can just be written with no check ? > > > > > Mostly see prores with Rec709, Rec601. Need to take a look for Rec2020. > Don't know if all value are authorized, doesn't use other colorspace, with > prores. > > Do you think it's better to only authorize few colorspace ? depends on what happens if "others" are stored. if the official decoders fail with a blank screen then its probably not a good idea to use such a value. If OTOH they ignore values they do not support then it may be ok. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB No human being will ever know the Truth, for even if they happen to say it by chance, they would not even known they had done so. -- Xenophanes signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel