Re: [FFmpeg-devel] [ANNOUNCE] upcoming vote: extra members for GA

[Reimar Döffinger]

Hi!

> On 9 Nov 2023, at 13:28, Anton Khirnov  wrote:
> 
> Quoting James Almer (2023-11-09 13:24:45)
>> Hendrik Leppkes and Reimar Döffinger are active, at least. I agree they 
>> should be in the GA.
> 
> Right, I did not mean to imply ALL the people in the above list are
> inactive. But many are.

Yes, I am still active and would be interested in having a voice/vote.
But I'll have to bluntly admit I do not have the time or the stomach
for many of the discussions on the list, which is why I've not acted
on this in the past.

Best regards,
Reimar
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] hevcdsp_idct_neon.S: Avoid unnecessary mov.

[Reimar Döffinger]

> On 26 Jul 2023, at 21:15, reimar.doeffin...@gmx.de wrote:
> 
> From: Reimar Döffinger 
> 
> ret can be given an argument instead.
> This is also consistent with how other assembler code
> in FFmpeg does it.

Now pushed.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] libaformat: fix incorrect handling of incomplete AVBPrint.

[Reimar Döffinger]

> On 27 Jul 2023, at 19:44, Nicolas George  wrote:
> 
> Reimar Döffinger (12023-07-27):
>> Thanks, sent a new version with that updated, plus a fix for a typo
>> in the commit message.
> 
> If it is all you have changed, then I do not think I need to look at it
> again.
> 
> I do not maintain most of the files you have changed, but I think you
> can go ahead.

Thanks, pushed it.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v4 0/4] Fix MSVC build without optimizations

[Reimar Döffinger]

> On 28 Jul 2023, at 12:40, Reimar Döffinger  wrote:
> 
> 
>> On 28 Jul 2023, at 03:37, L. E. Segovia  wrote:
>> 
>> Updated for 6.0, any constructive feedback will be appreciated.
> 
> Using #if means the code will not even be compile tested if the
> feature is disabled, this makes maintenance especially of rare
> features much harder (nevermind code readability).
> Also I believe we do not support compiling without optimizations
> even on primary platforms like Linux/gcc, so I'd find it very unlikely
> we'd want that for MSVC.
> At least there would have to be a very good argument why this is so
> important to have that we'd accept the drawbacks.

Since you asked for constructive feedback:
I assume the issue is missing symbols during linking?
If you really want this, why not create a file that provides dummy
symbols for all that are missing, concentrating the #if mess in
a single place and avoiding affecting any of the regular code, and thus
having no impact at all when compiling with optimizations.
Yes, it's likely to be a good bit of maintenance effort for those who
want to use it, but at least anyone not caring about this feature can
ignore it, so at least I would not have a reason to be against it.

Best regards,
Reimar
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v4 0/4] Fix MSVC build without optimizations

[Reimar Döffinger]

> On 28 Jul 2023, at 03:37, L. E. Segovia  wrote:
>
> Updated for 6.0, any constructive feedback will be appreciated.

Using #if means the code will not even be compile tested if the
feature is disabled, this makes maintenance especially of rare
features much harder (nevermind code readability).
Also I believe we do not support compiling without optimizations
even on primary platforms like Linux/gcc, so I'd find it very unlikely
we'd want that for MSVC.
At least there would have to be a very good argument why this is so
important to have that we'd accept the drawbacks.

Best regards,
Reimar
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] libaformat: fix incorrect handling of incomplete AVBPrint.

[Reimar Döffinger]

> On 27 Jul 2023, at 19:33, Nicolas George  wrote:
> 
> reimar.doeffin...@gmx.de (12023-07-23):
>> From: Reimar Döffinger 
>> 
>> Change some internal APIs a bit to make it harder to make
>> such mistakes.
>> In particular, have the read chunk functions return an error
>> when the result is incomplete.
>> This might be less flexible, but since there has been no
>> use-case for that so far, avoiding coding mistakes seems better.
>> Add a function to queue a AVBPrint directly 
>> (ff_subtitles_queue_insert_bprint).
>> Also fixes a leak in lrcdec when ff_subtitles_queue_insert fails.
>> ---
>> libavformat/assdec.c |  4 +++-
>> libavformat/lrcdec.c |  7 ++-
>> libavformat/mpsubdec.c   |  5 +++--
>> libavformat/realtextdec.c|  6 +-
>> libavformat/samidec.c|  6 +-
>> libavformat/srtdec.c |  4 +++-
>> libavformat/subtitles.c  | 17 +
>> libavformat/subtitles.h  | 14 --
>> libavformat/tedcaptionsdec.c |  2 +-
>> libavformat/webvttdec.c  |  4 +++-
>> 10 files changed, 54 insertions(+), 15 deletions(-)
> 
> Sorry I forgot I meant to review. These changes look for the best,
> except 
> 
>> +if (!av_bprint_is_complete(event)) return NULL;
> 
>> +if (i == INT_MAX) return AVERROR_INVALIDDATA;
> 
> … which do not follow the coding style.

Thanks, sent a new version with that updated, plus a fix for a typo
in the commit message.

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] libaformat: fix incorrect handling of incomplete AVBPrint.

[Reimar Döffinger]

> On 23 Jul 2023, at 14:00, reimar.doeffin...@gmx.de wrote:
> 
> From: Reimar Döffinger 
> 
> Change some internal APIs a bit to make it harder to make
> such mistakes.
> In particular, have the read chunk functions return an error
> when the result is incomplete.
> This might be less flexible, but since there has been no
> use-case for that so far, avoiding coding mistakes seems better.
> Add a function to queue a AVBPrint directly 
> (ff_subtitles_queue_insert_bprint).
> Also fixes a leak in lrcdec when ff_subtitles_queue_insert fails.

Please make any objections or need for more time to review known soon.
Otherwise I plan on pushing in a few days.

Best regards,
Reimar

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] hevcdsp_idct_neon.S: Avoid unnecessary mov.

[Reimar Döffinger]

> On 26 Jul 2023, at 21:43, Martin Storsjö  wrote:
> 
> On Wed, 26 Jul 2023, reimar.doeffin...@gmx.de wrote:
> 
>> From: Reimar Döffinger 
>> 
>> ret can be given an argument instead.
>> This is also consistent with how other assembler code
>> in FFmpeg does it.
>> ---
>> libavcodec/aarch64/hevcdsp_idct_neon.S | 6 ++
>> 1 file changed, 2 insertions(+), 4 deletions(-)
>> 
>> diff --git a/libavcodec/aarch64/hevcdsp_idct_neon.S 
>> b/libavcodec/aarch64/hevcdsp_idct_neon.S
>> index b7f23386a4..f7142c939c 100644
>> --- a/libavcodec/aarch64/hevcdsp_idct_neon.S
>> +++ b/libavcodec/aarch64/hevcdsp_idct_neon.S
>> @@ -617,8 +617,7 @@ function ff_hevc_idct_16x16_\bitdepth\()_neon, export=1
>> 
>>add  sp,  sp,  #640
>> -mov x30, x15
>> -ret
>> +ret x15
>> endfunc
>> .endm
>> @@ -814,8 +813,7 @@ function ff_hevc_idct_32x32_\bitdepth\()_neon, export=1
>> .endr
>> 
>>add sp,  sp,  #2432
>> -mov x30, x15
>> -ret
>> +ret x15
>> endfunc
>> .endm
> 
> LGTM, assuming checkasm still passes.

It does. Will push soon (on the assumption I still can...) if no objections.

Best regards,
Reimar
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] Replace br return with ret

[Reimar Döffinger]

> On 27 Jul 2023, at 15:55, Rémi Denis-Courmont  wrote:
> 
> Hi,
> 
> The use of RET vs BR also has microarchitectural side effects. AFAIU, RET 
> should always be paired with an earlier BL/BLR to avoid interfering with 
> branch prediction.
> 
> So depending on the circumstances, either one of these should be addressed:
> * Clarify that this is actually a function return , and RET should be used 
> anyway, regardless of BTI.
> * Keep BR and add BTI J landing pads where appropriate, if this wasn't really 
> a function return.

Yes BL and RET is best to match up.

For this function:
% git grep func_tr_32x4
libavcodec/aarch64/hevcdsp_idct_neon.S:function func_tr_32x4_\name
libavcodec/aarch64/hevcdsp_idct_neon.S:bl  
func_tr_32x4_firstpass
libavcodec/aarch64/hevcdsp_idct_neon.S:bl  
func_tr_32x4_secondpass_\bitdepth
libavcodec/arm/hevcdsp_idct_neon.S:function func_tr_32x4_\name
libavcodec/arm/hevcdsp_idct_neon.S:bl  
func_tr_32x4_firstpass
libavcodec/arm/hevcdsp_idct_neon.S:bl  
func_tr_32x4_secondpass_\bitdepth

It is always used with "bl", thus ret is also more correct from
that aspect.
Was your comment only on checking that, or did you mean that this should
be mentioned in the commit message?
(if you are wondering why the code did not use ret before, I guess it's
because it was ported from the 32-bit arm assembler and it slipped by code 
review)

Best regards,
Reimar
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avcodec/parser: Check next against buffer index

[Reimar Döffinger]

Hi!

> On 24 Jun 2023, at 21:14, Andreas Rheinhardt  
> wrote:
> 
> Michael Niedermayer:
>> Fixes: out of array access
>> Fixes: crash-0d640731c7da52415670eb47a2af701cbe2e1a3b
>> 
>> Found-by: Catena cyber 
>> Signed-off-by: Michael Niedermayer 
>> ---
>> libavcodec/parser.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/libavcodec/parser.c b/libavcodec/parser.c
>> index efc28b8918..db39e698ab 100644
>> --- a/libavcodec/parser.c
>> +++ b/libavcodec/parser.c
>> @@ -214,7 +214,7 @@ int ff_combine_frame(ParseContext *pc, int next,
>> for (; pc->overread > 0; pc->overread--)
>> pc->buffer[pc->index++] = pc->buffer[pc->overread_index++];
>> 
>> -if (next > *buf_size)
>> +if (next > *buf_size || (next < -pc->index && next != END_NOT_FOUND))
>> return AVERROR(EINVAL);
>> 
>> /* flush remaining if EOF */
> 
> Could you provide more details about this? E.g. which parser is this
> about at all? And how can we actually come in this situation at all?
> (Whenever I looked at ff_combine_frame() I do not really understand what
> its invariants are supposed to be.)

Yeah, when I looked at it I also felt like it has all kinds of
assumptions/preconditions without which it will break, but those
are not documented. Not really reviewable for correctness.
The change I proposed to fix the same issue was as below. But
note that it is not based on actual understanding, just that generally
index, overread_index and buf_size are updated together so I
thought it suspicious buf_size was not updated on realloc failure.
--- a/libavcodec/parser.c
+++ b/libavcodec/parser.c
@@ -252,6 +252,7 @@ int ff_combine_frame(ParseContext *pc, int next,
   AV_INPUT_BUFFER_PADDING_SIZE);
if (!new_buffer) {
av_log(NULL, AV_LOG_ERROR, "Failed to reallocate parser buffer to 
%d\n", next + pc->index + AV_INPUT_BUFFER_PADDING_SIZE);
+*buf_size =
pc->overread_index =
pc->index = 0;
return AVERROR(ENOMEM);

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] aarch64: Implement stack spilling in a consistent way.

[Reimar Döffinger]

Hi Martin,

> On 10 Oct 2022, at 23:29, Martin Storsjö  wrote:
> 
> On Sun, 9 Oct 2022, reimar.doeffin...@gmx.de wrote:
> 
>> From: Reimar Döffinger 
>> 
>> Currently it is done in several different ways, which
>> might cause needless dependencies or in case of
>> tx_float_neon.S is incorrect.
> 
> This looks reasonable to me, assuming that it passes fate. Do you want to 
> push it yourself, or do you want me to do it?

Thanks, I pushed it.
I had only run checkasm on it (I had made a couple of mistakes first), but it 
also passes fate.

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] tx_float_neon: Do not access outside stack.

[Reimar Döffinger]

> On 9 Oct 2022, at 16:11, Rémi Denis-Courmont  wrote:
> 
> Le sunnuntaina 9. lokakuuta 2022, 16.14.47 EEST Reimar Döffinger a écrit :
>> Use load/store instructions that modify sp to save
>> registers to stack, like it is done for all other
>> functions.
>> At least valgrind complains about the current code.
>> ---
>> libavutil/aarch64/tx_float_neon.S | 16 
>> 1 file changed, 8 insertions(+), 8 deletions(-)
>> 
>> diff --git a/libavutil/aarch64/tx_float_neon.S
>> b/libavutil/aarch64/tx_float_neon.S index 4126c3b812..4be93cc963 100644
>> --- a/libavutil/aarch64/tx_float_neon.S
>> +++ b/libavutil/aarch64/tx_float_neon.S
>> @@ -866,10 +866,10 @@ FFT16_FN ns_float, 1
>> 
>> .macro FFT32_FN name, no_perm
>> function ff_tx_fft32_\name\()_neon, export=1
>> -stp d8,  d9,  [sp, #-16]
>> -stp d10, d11, [sp, #-32]
>> -stp d12, d13, [sp, #-48]
>> -stp d14, d15, [sp, #-64]
>> +stp d8,  d9,  [sp, #-16]!
>> +stp d10, d11, [sp, #-16]!
>> +stp d12, d13, [sp, #-16]!
>> +stp d14, d15, [sp, #-16]!
> 
> While this fixes the ABI violation, it introduces multiple data dependencies 
> on 
> stack pointer due to write-back.

That is true in principle, this is not done consistently at all.

> The idiomatic way to do this is to allocate the entire needed stack space in 
> the first store / last load, and use positive offsets elsewhence.

Are you sure this is really relevant at all, considering it's so rarely done in 
the code?
I'm fine looking into doing that consistently if that's what it takes, but I 
don't think it's good to make this the only function in the file (if not the 
whole lib) that does it this way.
If we want the idiomatic way to be used, we should
update all the code so people get the right idea.
(I'll note that some places might be tricky, there seems to be code that 
updates sp back and forth in a loop which looks like it might be for no reason, 
or there is a weird reason - check SR_TRANSFORM_DEF in the same file).

Just to clarify the details, then end result should look like this?
-stp d8,  d9,  [sp, #-16]
-stp d10, d11, [sp, #-32]
-stp d12, d13, [sp, #-48]
-stp d14, d15, [sp, #-64]
+stp d14, d15, [sp, #-16*4]!
+stp d8,  d9,  [sp, #16*3]
+stp d10, d11, [sp, #16*2]
+stp d12, d13, [sp, #16]

Or something slightly different?

Best regards,
Reimar
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH] tx_float_neon: Do not access outside stack.

[Reimar Döffinger]

Use load/store instructions that modify sp to save
registers to stack, like it is done for all other
functions.
At least valgrind complains about the current code.
---
 libavutil/aarch64/tx_float_neon.S | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/libavutil/aarch64/tx_float_neon.S 
b/libavutil/aarch64/tx_float_neon.S
index 4126c3b812..4be93cc963 100644
--- a/libavutil/aarch64/tx_float_neon.S
+++ b/libavutil/aarch64/tx_float_neon.S
@@ -866,10 +866,10 @@ FFT16_FN ns_float, 1
 
 .macro FFT32_FN name, no_perm
 function ff_tx_fft32_\name\()_neon, export=1
-stp d8,  d9,  [sp, #-16]
-stp d10, d11, [sp, #-32]
-stp d12, d13, [sp, #-48]
-stp d14, d15, [sp, #-64]
+stp d8,  d9,  [sp, #-16]!
+stp d10, d11, [sp, #-16]!
+stp d12, d13, [sp, #-16]!
+stp d14, d15, [sp, #-16]!
 
 LOAD_SUBADD
 SETUP_SR_RECOMB 32, x7, x8, x9
@@ -911,10 +911,10 @@ function ff_tx_fft32_\name\()_neon, export=1
 zip2v31.2d, v11.2d, v15.2d
 st1 { v28.4s, v29.4s, v30.4s, v31.4s }, [x1]
 
-ldp d14, d15, [sp, #-64]
-ldp d12, d13, [sp, #-48]
-ldp d10, d11, [sp, #-32]
-ldp d8,  d9,  [sp, #-16]
+ldp d14, d15, [sp], #16
+ldp d12, d13, [sp], #16
+ldp d10, d11, [sp], #16
+ldp d8,  d9,  [sp], #16
 
 ret
 endfunc
-- 
2.37.2

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] configure: stop allowing disabling lzo

[Reimar Döffinger]

> On 26 Feb 2022, at 16:33, James Almer  wrote:
> 
> The module is now always compiled in.

Thanks, both patches in this series or the alternative patch are fine with me.
Only possible downside I could think of is if there is any use-case why someone 
would want the matroska decoder to specifically NOT have LZO support.

Best regards,
Reimar

P.S.: If it's not too much effort I do appreciate a CC on patches for things 
where I am mentioned in MAINTAINERS as the list is a bit high-traffic for me
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] DRM decryption with FFmpeg

[Reimar Döffinger]

> On 9 Mar 2021, at 10:44, zsugabubus  
> wrote:
> 
> Hi all,
> 
> I was not able to find any patches or mails, only two open tickets that
> mention DRM decryption in some way:
> 
> https://trac.ffmpeg.org/ticket/1793
> https://trac.ffmpeg.org/ticket/1800
> 
> ...however there were no updates for years. I do not know it is because
> the lack of interest or the lack of manpower but I would like to help
> making some progress in this topic.
> 
> There were any plans/ideas about how DRM encrypted content should be
> handled (other than those tickets above)? I mean API or such. Or was
> that all the discussion about it and it is totally okay if somebody just
> submits some code for RFC?


“DRM” is problematic word to choose, since it’s unclear what you might mean with
that (DRM itself in the meaning of somehow trying to allow users to decode the
data only in certain ways and contexts is not possible to implement in 
OpenSource
that is worth the name), not to mention potential legal questions.
If the question is just about various encryption methods and supporting 
decrypting them,
there is an API to set a decryption key, supported by MXF and it seems some
stuff for MOV as well, maybe more I did not see.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] configure: Fix bashism in openal check. (was: [PATCH] Bugfix for #9135)

[Reimar Döffinger]

> 
> I have never come around to setting up postfix for forwarding mails 
> externally on my current system. So, for now, I am using the second best 
> solution. Please see the attachment.

Maybe not worth the effort, but FYI git can communicate directly with
a SMTP server nowadays, and msmtmp is a simpler solution if all
you want is a simple forwarding sendmail (not normally
needed for git anymore though).
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH V3 3/5] libavformat/protocols.c: fix build warning for [-Wdiscarded-qualifiers]

[Reimar Döffinger]

> On 27 Feb 2021, at 09:14, Guo, Yejun  wrote:
> 
> 
> 
>> -Original Message-
>> From: ffmpeg-devel  On Behalf Of Paul B
>> Mahol
>> Sent: 2021年2月26日 19:37
>> To: FFmpeg development discussions and patches 
>> Cc: Guo, Yejun 
>> Subject: Re: [FFmpeg-devel] [PATCH V3 3/5] libavformat/protocols.c: fix build
>> warning for [-Wdiscarded-qualifiers]
>> 
>> look at same/similar patches like yours that have been already rejected.
> 
> thanks for the info.
> 
> My motivation to fix the build warnings is that: I have several patches caught
> by patchwork with build warnings that I missed. I'd like to avoid it. And the 
> best method,
> IMO, is to add '-Wall -Werror' for the build option.

I think that is a bad idea, compilers add new warnings all the time, many of 
them
unreliable or of questionable quality/usefulness.
What you end up with using -Werror is a project that constantly fails to build 
and gets
a reputation for low quality.
However there may well be specific, reliable warning types for which we
should add an error specifically, as we already do now for e.g.
-Werror=implicit-function-declaration
There might also be other options to improve things and ensure relevant
warnings are visible and addressed, but I’ve yet to
see a -Werror have convincing effects (for example one effect it usually
has is that people have to stick to “supported” compiler versions to avoid
constant issues with build failures, reducing testing and actually decreasing
code quality instead of increasing it).
Turning it around, using -Werror successfully would mean testing with all
compiler versions regularly including pre-release versions and ensuring
any issues are fixed immediately. Which I don’t think is an approach
that would work for FFmpeg.

> Such option can also help to find
> potential issues in the code. This is the reason that I plan to fix the build 
> warnings
> one by one together with community.

I believe many are likely to be fixable in a way that most developers
would be quite happy with, even consider it an improvement beyond
just the warning/bug fix.
It’ll just take a lot more effort to find those types of fixes.
After all if it was that simple there could just be an option for
the compiler to fix them itself, the reason there isn’t is because
it would likely end very badly.

Best regards,
Reimar
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH V2 1/7] libavdevice/v4l2.c: fix build warning for [-Wformat-truncation=]

[Reimar Döffinger]

> On 27 Feb 2021, at 06:37, Guo, Yejun  wrote:
> 
> 
>> -Original Message-
>> From: ffmpeg-devel  On Behalf Of Reimar
>> D?ffinger
>>> 
> 
> For the code in this function, max length of file name is fixed, see the code 
> below.
> Anyway, it still increases the on-stack variable size which might have 
> potential security
> issue.
> 
>snprintf(device_name, sizeof(device_name), "/dev/%s", entry->d_name);
> 
> 'man readdir' shows:
>   struct dirent {
>   ino_t  d_ino;   /* Inode number */
>   off_t  d_off;   /* Not an offset; see below */
>   unsigned short d_reclen;/* Length of this record */
>   unsigned char  d_type;  /* Type of file; not supported
>  by all filesystem types */
>   char   d_name[256]; /* Null-terminated filename */
>   };

The size is not standardised.
E.g. the OSX manpage has:
 struct dirent { /* when _DARWIN_FEATURE_64_BIT_INODE is defined */
 ino_t  d_fileno; /* file number of entry */
 __uint64_t d_seekoff;/* seek offset (optional, used by servers)
 */
 __uint16_t d_reclen; /* length of this record */
 __uint16_t d_namlen; /* length of string in d_name */
 __uint8_t  d_type;   /* file type, see below */
 chard_name[1024];/* name must be no longer than this */
 };

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH V2 1/7] libavdevice/v4l2.c: fix build warning for [-Wformat-truncation=]

[Reimar Döffinger]

> On 25 Feb 2021, at 18:52, Chad Fraleigh  wrote:
> 
> On 2/24/2021 10:38 PM, Guo, Yejun wrote:
>>  libavdevice/v4l2.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c
>> index 365bacd771..e11d10d20e 100644
>> --- a/libavdevice/v4l2.c
>> +++ b/libavdevice/v4l2.c
>> @@ -1046,7 +1046,7 @@ static int v4l2_get_device_list(AVFormatContext *ctx, 
>> AVDeviceInfoList *device_l
>>  return ret;
>>  }
>>  while ((entry = readdir(dir))) {
>> -char device_name[256];
>> +char device_name[512];
> 
> Is there a portable path max constant that would be better for cases like 
> this, rather than just picking another arbitrary buffer size?

There is PATH_MAX/MAX_PATH, however the problems are:
1) They are not necessarily a strict limit on path length, so no guarantee all 
file names fit
2) Even if there is such a guarantee, that only applies to VALID file names, 
however the files here are user input ans not necessarily valid, so using those 
defines does not fix things anyway

Speaking generally I have doubts that these patches actually IMPROVE security 
and don’t make it actually worse.
On the plus side I see:
- they fix truncations in those specific cases
On the minus side I see:
- file names can clearly be longer, so there is still a truncation issue even 
after these patches, truncation just happens elsewhere and at larger lengths
- the warning is removed, so now there is nothing that reminds developers of 
this issue existing
- it relies on “magic constants” that can easily become outdated and 
re-introduce the issue again at any time
- there is as far as I can tell no evidence that any of these truncations cause 
actual issues, or if so in which circumstances, so there is no evidence of 
benefit. However as with all changes there is a risk of introducing new bugs
- size of on-stack variables are increased which may decrease effectiveness of 
stack protection

As a result personally I am mostly worried about these patches if they are 
applied without a deep security review of each and ensuring the issues are 
understood and thoroughly fixed.
I before suggested using av_asprintf, which does at least permanently solve the 
truncation issue without magic constants and eliminates on-stack arrays, 
however I should also add that it might create a allocation failure issue, 
which then opens up the whole “how to handle allocation failure without 
introducing a even more tricky security issue”.
So I am afraid that these issues will be annoyingly costly to fix, and I am not 
sure how big the desire is to do so...

Best regards,
Reimar
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH V2 6/7] libavformat/smoothstreamingenc.c: fix build warning for [-Wformat-truncation=]

[Reimar Döffinger]

> On 25 Feb 2021, at 07:38, Guo, Yejun  wrote:
> --- a/libavformat/smoothstreamingenc.c
> +++ b/libavformat/smoothstreamingenc.c
> @@ -501,7 +501,7 @@ static int ism_flush(AVFormatContext *s, int final)
> 
> for (i = 0; i < s->nb_streams; i++) {
> OutputStream *os = >streams[i];
> -char filename[1024], target_filename[1024], header_filename[1024], 
> curr_dirname[1024];
> +char filename[2048], target_filename[2048], header_filename[2048], 
> curr_dirname[1024];
> int64_t size;
> int64_t start_ts, duration, moof_size;
> if (!os->packets_written)

IMO some of these allocations are getting a bit too large for the stack
(multi-page stack allocations weaken security measures even if the large
arrays themselves do not overflow).
And no matter what size you put, there’s always a larger filename possible
where it breaks, so it feels like just warning polishing with marginal
real benefit.
Why not use av_asprintf, then at least the problem is actually solved for real?
I don’t see that this code is performance relevant, so the only reason to
put these onto stack is being too lazy to do the memory management, which
I think is a fairly weak argument.

Best regards,
Reimar
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] Ticket #8750 Add inline function for the vec_xl intrinsic in non-VSX environments

[Reimar Döffinger]

Hi!

> On 24 Jan 2021, at 17:35, Andriy Gelman  wrote:
> 
> On Sat, 07. Nov 16:31, Andriy Gelman wrote:
>> On Sat, 31. Oct 10:17, Andriy Gelman wrote:
>>> On Fri, 16. Oct 00:02, Andriy Gelman wrote:
 On Fri, 09. Oct 20:17, Andriy Gelman wrote:
> From: Chip Kerchner 
> 
> ---
> libswscale/ppc/yuv2rgb_altivec.c | 10 ++
> 1 file changed, 10 insertions(+)
> 
> diff --git a/libswscale/ppc/yuv2rgb_altivec.c 
> b/libswscale/ppc/yuv2rgb_altivec.c
> index 536545293d..930ef6b98f 100644
> --- a/libswscale/ppc/yuv2rgb_altivec.c
> +++ b/libswscale/ppc/yuv2rgb_altivec.c
> @@ -283,6 +283,16 @@ static inline void cvtyuvtoRGB(SwsContext *c, vector 
> signed short Y,
>  * 
> --
>  */
> 
> +#if !HAVE_VSX
> +static inline vector unsigned char vec_xl(signed long long offset, const 
> ubyte *addr)
> +{
> +const vector unsigned char *v_addr = (const vector unsigned char *) 
> (addr + offset);
> +vector unsigned char align_perm = vec_lvsl(offset, addr);
> +
> +return (vector unsigned char) vec_perm(v_addr[0], v_addr[1], 
> align_perm);
> +}
> +#endif /* !HAVE_VSX */
> +
> #define DEFCSP420_CVT(name, out_pixels)   
> \
> static int altivec_ ## name(SwsContext *c, const unsigned char **in,  
> \
> int *instrides, int srcSliceY, int srcSliceH, 
> \
 
 Ping.
 This patch fixes PPC64 build on patchwork.
 
>>> 
>>> ping
>>> 
>> 
>> ping 
>> 
> 
> I asked Reimar to have a look at the patch. He didn't have a link to original
> post, so I'm forwarding his feedback: 
> 
> - a few comments sure would be good
> - the function likely should be always_inline
> - the offset is always 0 in the code, so that could be optimized
> - I am not sure if the addresses even can be unaligned (the whole magic code 
> is about fixing up unaligned loads since altivec simply ignores the low bits 
> of the address, but it looks like the x86 asm also assumes unaligned)
> - the extra load needed to fix the alignment can read outside the bounds of 
> the buffer I think? Especially for chroma and if the load is aligned. Though 
> chroma seems to have an issue already today...
> 

I had a look at the code before the vec_xl was introduced, and besides the 
unnecessary extra offset it seems it was pretty much like this.
So worst case this patch would restore the behaviour to before the vec_xl 
patch, which I
guess is a reasonable argument to merge it whether it’s perfect or not.
Personally I would have added a vecload (dropping the offset argument) or 
similar function
that either does vec_xl or this, instead of introducing a “fake” vec_xl 
function,
but that is just nitpicking I guess…

Best regards,
Reimar

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] libavcodec/hevcdsp: port SIMD idct functions from 32-bit.

[Reimar Döffinger]

Hi Martin!

> On 10 Feb 2021, at 22:53, Martin Storsjö  wrote:
> 
> +.macro idct_16x16 bitdepth
> +function ff_hevc_idct_16x16_\bitdepth\()_neon, export=1
> +//r0 - coeffs
> +mov x15, lr
> +
 Binutils doesn't recognize "lr" as alias for x30
>>> It didn’t have an issue in the Debian unstable VM?
>>> That seems like the kind of workaround where it would be
>>> better to leave a comment with more info, if you know
>>> what exactly is affected.
>> 
>> Binutils 2.28 doesn't recognize "lr" while 2.30 does, it seems.
>> 
>> FWIW, all the existing aarch64 assembly just uses "x30" to refer to this 
>> register, none of it uses "lr".
> 
> Do you want to follow up on this patch? IIRC changing it to use "x30" instead 
> of "lr" was the only blocker from my point of view (and the add_residual 
> patch that goes on top of it was mostly fine as well)?

Sorry, I forgot about that comment when I sent the last revision.
Josh has been doing some polishing of these patches, so unless I hear
otherwise I’ll assume he’s volunteering to do these minor fixes
(thanks in advance), otherwise we just end up stepping on each other’s toes.
But I am around in principle and will if necessary help out getting it merged.

Best regards,
Reimar
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH]lsws/ppc/yuv2rgb: Fix transparency converting from yuv->rgb32

[Reimar Döffinger]

> On 23 Jan 2021, at 19:35, Carl Eugen Hoyos  wrote:
> 
> 
> New patch attached, thank you!
> 

Looks good to me.
I don’t know if I didn’t test the original change properly
or if compiler behaviour has changed/is different,
but this way should be more correct either way.

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH]lsws/ppc/yuv2rgb: Fix transparency converting from yuv->rgb32

[Reimar Döffinger]

Hi!

> On 23 Jan 2021, at 17:31, Carl Eugen Hoyos  wrote:
> Attached patch fixes ticket #9077 for me.
> 
> Please comment, Carl Eugen
> <0001-lsws-ppc-yuv2rgb-Fix-transparency-converting-from-yu.patch>

Unfortunately I can’t test anything myself anymore since
I don’t have any devices with VGA input anymore (and connecting
the PPC MacMini via HDMI never worked reliably).
But checking the surrounding code it might make sense to check
if vec_splat(…{255}, 0) also works, or maybe even generates
better code.
The other cases where a non-0 constant (128) is needed in the
whole vector use vec_splat instead of duplicating the value,
so I assume there is a reason.
In this case actually even more, since the code is generic and
we might not actually know just how many 255s exactly we need
to put into that array?

Best regards,
Reimar
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH] configure: add fallback to $arch in msvc assembler check.

[Reimar Döffinger]

Setting the defaults for $arch happens only later, so
the current code would not set AS correctly if --arch
was not specified on the command-line.
Fix it by adding an explicit fallback to $arch_default.
---
 configure | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure b/configure
index 54fbbd6b5f..df298b4b9b 100755
--- a/configure
+++ b/configure
@@ -4268,7 +4268,7 @@ case "$toolchain" in
 ld_default="$source_path/compat/windows/mslink"
 nm_default="dumpbin.exe -symbols"
 ar_default="lib.exe"
-case "$arch" in
+case "${arch:-$arch_default}" in
 aarch64|arm64)
 as_default="armasm64.exe"
 ;;
--
2.30.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] libavcodec/hevcdsp: port SIMD idct functions from 32-bit.

[Reimar Döffinger]

> On 15 Jan 2021, at 23:55, Martin Storsjö  wrote:
> 
> On Tue, 12 Jan 2021, reimar.doeffin...@gmx.de wrote:
> 
>> create mode 100644 libavcodec/aarch64/hevcdsp_idct_neon.S
>> create mode 100644 libavcodec/aarch64/hevcdsp_init_aarch64.c
> 
> This patch fails checkasm

Fixed, one mis-translated coefficient index...

>> 
>> +.macro fixsqrshrn d, dt, n, m
>> +  .ifc \dt, .8H
>> +sqrshrn2\d\dt, \n\().4S, \m
>> +  .else
>> +sqrshrn \n\().4H, \n\().4S, \m
>> +mov \d\().D[0], \n\().D[0]
>> +  .endif
>> +.endm
> 
> Is this to set the lower half of the dest register, without wiping the upper 
> part? It looks a bit clumsy (and stall-prone on in-order cores), but I guess 
> it's not easy to do things differently without rewriting the higher level 
> structure?

Did not have a good idea, but I was also aiming for keeping the structure of 
the 32-bit and 64-bit code similar.
In particular since I did not know about checkasm and expected some further 
painful debug.

> 
>> +.macro transpose_8x8 r0, r1, r2, r3, r4, r5, r6, r7
>> +transpose8_4x4  \r0, \r1, \r2, \r3
>> +transpose8_4x4  \r4, \r5, \r6, \r7
>> +.endm
>> +
> 
> There's a bunch of existing transposes in libavcodec/aarch64/neon.S - can 
> they be used? Not that they're rocket science, though... And I see that the 
> existing arm version also has got its own transpose macros.
> 
> If it's inconvenient to use shared macros, this is fine.

They are different and seem to not be documented, so it would take some
time to figure out how to replace them.
There’s also a bit of a question if I’d want to give up alignment
with the 32-bit code just yet.


> 
>> +// Transpose each 4x4 block, and swap how d4-d7 and d8-d11 are used.
>> +// Layout before:
>> +// d0  d1
>> +// d2  d3
>> +// d4  d5
>> +// d6  d7
>> +// d8  d9
>> +// d10 d11
>> +// d12 d13
>> +// d14 d15
> 
> These layouts don't look like they're up to date for the aarch64 version?

Removed in new version as it seems not that useful.

>> 
>> +vst1.s32{\in7}, [r3, :128]
>> +.endm
> 
> This is left behind untranslated (and thus unused)

Removed. I am not sure if that means it’s also unused in the 32-bit version.

>> +
>> +movrel  x1, trans + 16
> 
> The movrel macro has got a separate third optional argument for the offset, 
> so write this "movrel x1, trans, 16". (Older versions of llvm are picky with 
> the symbol offset syntax and break with this, as the macro right now adds its 
> own implicit +(0) here. If you pass the offset in the macro parameter, all 
> the offsets get passed within the parentheses.

Changed.

>> +.macro idct_16x16 bitdepth
>> +function ff_hevc_idct_16x16_\bitdepth\()_neon, export=1
>> +//r0 - coeffs
>> +mov x15, lr
>> +
> 
> Binutils doesn't recognize "lr" as alias for x30

It didn’t have an issue in the Debian unstable VM?
That seems like the kind of workaround where it would be
better to leave a comment with more info, if you know
what exactly is affected.

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] configure: Set MSVC as_default later.

[Reimar Döffinger]

> On 15 Jan 2021, at 23:25, Martin Storsjö  wrote:
> 
> On Fri, 15 Jan 2021, reimar.doeffin...@gmx.de wrote:
> 
>> From: Reimar Döffinger 
>> 
>> It would get immediately overridden to $cc, which in case
>> of gas-preprocessor missing would result in it trying
>> to use cl.exe for asm files instead of erroring out.
>> This is because cl.exe does not fail but just print a warning
>> when it is given a file it does not know what to do with it...
> 
> As this setup seems to work fine in the setups I've tried, can you think of 
> why it's overwritten with $cc in your cases?
> 
> With the line
>: ${as_default:=$cc}
> it only sets as_default to $cc if $as_default is empty.

Actually after a few debug prints it’s clear what actually happens:
$arch is not set at that point unless specified on command-line.
Not sure if it’s reasonable to just check arch_default as fallback or such?

> You can't really do that here. Probe_cc only should set the existing set of 
> _type/_ident/_ldflags/_cflags* etc variables, which are picked up by the 
> caller of probe_cc. probe_cc is called separately for both host and target 
> compilers, so if e.g. cross compiling, with MSVC as host compiler, with a 
> different compiler for the target, this wouldn't do the right thing.

Then the armcc logic in there is broken I guess?

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] libswscale/aarch64/hscale.S: Support more bit-depth variants.

[Reimar Döffinger]

> On 15 Jan 2021, at 22:10, Martin Storsjö  wrote:
> 
> On Mon, 11 Jan 2021, reimar.doeffin...@gmx.de wrote:
> 
>> From: Reimar Döffinger 
>> 
>> Trivially expand hscale assembler to support > 8 bit formats
>> both for input and output.
>> 16-bit input is not supported as I am not certain how to
>> get sufficient test coverage.
>> ---
>> libswscale/aarch64/hscale.S  | 53 ++--
>> libswscale/aarch64/swscale.c | 49 +++--
>> 2 files changed, 85 insertions(+), 17 deletions(-)
>> 
>> diff --git a/libswscale/aarch64/hscale.S b/libswscale/aarch64/hscale.S
>> index af55ffe2b7..3b42d39dac 100644
>> --- a/libswscale/aarch64/hscale.S
>> +++ b/libswscale/aarch64/hscale.S
>> @@ -20,7 +20,11 @@
>> #include "libavutil/aarch64/asm.S"
>> -function ff_hscale_8_to_15_neon, export=1
>> +.macro hscale srcbits, dstbits, ldt, lds, c
>> +function ff_hscale_\srcbits\()_to_\dstbits\()_neon, export=1
>> +.if \dstbits >= 16
>> +moviv20.4S, #(0x1 << (\dstbits - 16)), msl #16
>> +.endif
>>sbfiz   x7, x6, #1, #32   // filterSize*2 (*2 
>> because int16)
>> 1:  ldr w8, [x5], #4 // filterPos[idx]
>>ldr w0, [x5], #4  // filterPos[idx + 1]
>> @@ -34,30 +38,30 @@ function ff_hscale_8_to_15_neon, export=1
>>moviv1.2D, #0 // val sum part 2 (for 
>> dst[1])
>>moviv2.2D, #0 // val sum part 3 (for 
>> dst[2])
>>moviv3.2D, #0 // val sum part 4 (for 
>> dst[3])
>> -add x17, x3, w8, UXTW   // srcp + filterPos[0]
>> -add x8,  x3, w0, UXTW   // srcp + filterPos[1]
>> -add x0, x3, w11, UXTW   // srcp + filterPos[2]
>> -add x11, x3, w9, UXTW   // srcp + filterPos[3]
>> +add x17, x3, w8, UXTW #!!(\srcbits > 8) // srcp + 
>> filterPos[0]
>> +add x8,  x3, w0, UXTW #!!(\srcbits > 8) // srcp + 
>> filterPos[1]
>> +add x0, x3, w11, UXTW #!!(\srcbits > 8) // srcp + 
>> filterPos[2]
>> +add x11, x3, w9, UXTW #!!(\srcbits > 8) // srcp + 
>> filterPos[3]
> 
> This construct breaks with llvm's assembler and armasm64 - they don't 
> evaluate !! here, ending up with errors like these:
> 
> :19:31: error: expected integer shift amount
>add x8, x3, w0, UXTW #!!(8 > 8)
> 
> libswscale\aarch64\hscale.o.asm(3093) : error A2007: wrong operand type for 
> :LNOT:
>add x17, x3, w8, UXTW #!!0
> 
> While less elegant, I guess this can be handled easily by adding a macro 
> parameter that represents the evaluated value of \srcbits > 8.

It’s actually easier. I just need 0 or 1 here, the thing I was missing
that caused me to add the !! is that a true condition does not evaluate
to 1 but to -1, so (\srcbits <= 8) + 1 works with both.


>>mov w15, w6   // filterSize counter
>> -2:  ld1 {v4.8B}, [x17], #8  // srcp[filterPos[0] + 
>> {0..7}]
>> +2:  ld1 {v4.\ldt}, [x17], \lds // srcp[filterPos[0] + 
>> {0..7}]
>>ld1 {v5.8H}, [x16], #16   // load 8x16-bit filter 
>> values, part 1
>> -ld1 {v6.8B}, [x8], #8   // srcp[filterPos[1] + 
>> {0..7}]
>> +ld1 {v6.\ldt}, [x8], \lds   // srcp[filterPos[1] + 
>> {0..7}]
>>ld1 {v7.8H}, [x12], #16   // load 8x16-bit at 
>> filter+filterSize
>> -uxtlv4.8H, v4.8B// unpack part 1 to 
>> 16-bit
>> +\c\cuxtlv4.8H, v4.8B// unpack part 1 to 
>> 16-bit
> 
> With gas-preprocessor and armasm64, // isn't considered a comment char by 
> armasm64, only by the C preprocessor invoked by gas-preprocessor, so it 
> doesn't strip out these lines properly. I guess one could add more code to 
> gas-preprocessor to handle that, but that's probably a bit overkill...

That is a whole another fun topic, I’ll be sending patches related to 
gas-preprocessor.
Admittedly might consider some of the issues my fault for not cross-compiling 
and also preferring WSL over msys2 or such...
And then I might get around to addressing the rest.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] Add support for "omp simd" pragma.

[Reimar Döffinger]

> If building with MSVC tools, yes you're right that armasm.exe/armasm64.exe 
> takes a different syntax. But the gas-preprocessor tool (which is picked up 
> automatically by our configure, one just needs to make sure it's available) 
> handles expanding all the macros and rewriting directives into the armasm 
> form, and feeding it to the armasm tools. Works fine and have done so for 
> years. There's even a wiki page which tries to explain how to do it (although 
> it's probably outdated in some aspects), see 
> https://trac.ffmpeg.org/wiki/CompilationGuide/WinRT.
> 

I went with the instructions in doc/platform.texi and that did not work at all,
It even tried to use cl.exe to compile the assembler files!

> All of these run with full assembly optimizations enabled. So please don't 
> tell me that our assembly doesn't work on windows on arm, because it does, 
> and it has for years.
> 

My apologies. I’ll correct to: it doesn’t work using the instructions
shipping with the source code (as far as I can tell) :)

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] Add support for "omp simd" pragma.

[Reimar Döffinger]

> On 12 Jan 2021, at 21:46, Lynne  wrote:
> 
> Jan 12, 2021, 19:28 by reimar.doeffin...@gmx.de:
> 
>> It’s almost 20%. At least for this combination of
>> codec and stream a large amount of time is spend in
>> non-DSP functions, so even hand-written assembler
>> won’t give you huge gains.
>> 
> It's non-guaranteed 20% on a single system. It could change, and it could very
> well mess up like gcc does with autovectorization, which we still explicitly 
> disable
> because FATE fails (-fno-tree-vectorize, and I was the one who sent an RFC to
> try to undo it somewhat recently. Even though it was an RFC the reaction from 
> devs
> was quite cold).

Oh, thanks for the reminder, I thought that was gone because it seems
it’s not used for clang, and MPlayer does not seem to set that.
I need to compare it, however the problem with the auto-vectorization
is exactly that the compiler will try to apply it to everything,
which has at least 2 issues:
1) it gigantically increases the risk for bugs when it's every
single loop instead of loops that we already wrote assembler for
somewhere.
2) it will quite often make things worse, by vectorizing loops
that are rarely iterated over more than a few times (and it
needs to handle a whole lot of code to handle loop counts not
a multiple of vector size) - because all too often the compiler
can only take a wild guess if “width” is usually 1 or 1920,
while we DO know.

>>> its definitely something the compiler should
>>> be able to decide on its own,
>>> 
>> 
>> So you object to unlikely() macros as well?
>> It’s really just giving the compiler a hint it should try, though I admit 
>> the configure part makes it
>> look otherwise.
>> 
> I'm more against the macro and changes to the code itself. If you can make it
> work without adding a macro to individual loops or the likes of 
> av_cold/av_hot or
> any other changes to the code, I'll be more welcoming.

I expect that will just run into the same issue as the tree-vectorize...

> I really _hate_ compiler hints. Take a look at the upipe source code to see 
> what
> a cthulian monstrosity made of hint flags looks like. Every single branch had
> a cold/hot macro and it was the project's coding style. It's completely 
> irredeemable.

I guess my suggested solution would be to require proof of
clearly measurable performance benefit.
But I see the point that if it gets “randomly” added to loops
that might turn out quite a mess.

>>> Most of the loops this is added to are trivially SIMDable.
>>> 
>> 
>> How many hours of effort do you consider “trivial”?
>> Especially if it’s someone not experienced?
>> It might be fairly trivial with intrinsics, however
>> many of your counter-arguments also apply
>> to intrinsics (and to a degree inline assembly).
>> That’s btw not just a rhetorical question because
>> I’m pretty sure I am not going to all the trouble
>> to port more of the arm 32-bit assembler functions
>> since it’s a huge PITA, and I was wondering if there
>> was a point to even have a try with intrinsics...
>> 
> Intrinsics and inline assembly are a whole different thing than magic
> macros that tell and force the compiler what a well written compiler
> should already very well know about.

There are no well written compilers, in a way ;)
I would also argue that most of what intrinsics do,
such a compiler should figure out on its own, too.
And the first time I tried intrinsics they slowed the
loop down by a factor 2 because the compiler stored and
loaded the value to stack between every intrinsic,
so it’s not like they are not without problems.
But I was actually thinking that it might be somewhat
interesting to have a kind of “generic SIMD intrinsics”.
Though I think I read that such a thing has already be
tried, so it might just be wasted time.

> I already said all that can be said here: this will halt efforts on actually
> optimizing the code in exchange for naive trust in compilers.

I’m not sure it will discourage it more than having to write
the optimizations over and over, for Armv7 NEON, for Armv8 Linux,
for Armv8 Windows, then SVE/SVE2, who knows maybe Armv9
will also need a rewrite.
SSE2, AVX256, AVX512 for x86, so much stuff never gets ported
to the new versions.
I’d also claim anyone naively trusting in compilers is unlikely
to write SIMD optimizations either way :)

> New platforms will be stuck at scalar performance anyway until
> the compilers for the arch are smart enough to deal with vectorization.

That seems to happen a long time before someone gets around to
optimising FFmpeg though.
This is particularly true when it’s a new OS and not CPU architecture
platform.
For example macOS we are lucky enough that the assembler etc. are
largely compatible to Linux.
But for Windows-on-Arm there is no GNU assembler, and the Microsoft
assembler needs a completely different syntax, so even the assembly
we DO have just doesn’t work.

Anyway, thanks for the discussion.
I still think the situation with SIMD optimizations 

Re: [FFmpeg-devel] [PATCH] Add support for "omp simd" pragma.

[Reimar Döffinger]

> On 12 Jan 2021, at 19:52, Soft Works  wrote:
> 
> 
> 
>> -Original Message-
>> From: ffmpeg-devel  On Behalf Of
>> reimar.doeffin...@gmx.de
>> Sent: Sunday, January 10, 2021 5:44 PM
>> To: ffmpeg-devel@ffmpeg.org
>> Cc: Reimar Döffinger 
>> Subject: [FFmpeg-devel] [PATCH] Add support for "omp simd" pragma.
>> 
>> From: Reimar Döffinger 
>> 
>> This requests loops to be vectorized using SIMD instructions.
>> The performance increase is far from hand-optimized assembly but still
>> significant over the plain C version.
>> Typical values are a 2-4x speedup where a hand-written version would
>> achieve 4x-10x.
>> So it is far from a replacement, however some architures will get hand-
>> written assembler quite late or not at all, and this is a good improvement 
>> for
>> a trivial amount of work.
>> The cause, besides the compiler being a compiler, is usually that it does not
>> manage to use saturating instructions and thus has to use 32-bit operations
>> where actually saturating 16-bit operations would be sufficient.
>> Other causes are for example the av_clip functions that are not ideal for
>> vectorization (and even as scalar code not optimal for any modern CPU that
>> has either CSEL or MAX/MIN instructions).
>> And of course this only works for relatively simple loops, the IDCT functions
>> for example seemed not possible to optimize that way.
> 
> ...
> 
>> +if enabled openmp_simd; then
>> +ompopt="-fopenmp"
>> +if ! test_cflags $ompopt ; then
>> +test_cflags -Xpreprocessor -fopenmp && ompopt="-Xpreprocessor -
>> fopenmp"
> 
> Isn't it sufficient to specify -fopenmp-simd instead of -fopenmp for this 
> patch?

I think so, I just didn’t know/even expect that option to exist!
Thanks a lot for the tip!

> As OMP SIMD is the only openmp feature that is used, there's no need to link
> to the openmp lib. 


That it doesn’t do anyway because -fopenmp is not in the linker flags,
but I admit that was a bit of a hacky solution.

Thanks,
Reimar
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2] Replace arrays of pointers to strings by arrays of strings

[Reimar Döffinger]

> On 12 Jan 2021, at 02:41, Andreas Rheinhardt  
> wrote:

> Of course I am all ears for how to make it clear that someone who
> modifies the strings also needs to check the array dimensions.

I think I kind of agree with the other comments, this would/should
rather have to be something that can be checked in an automated way.
In principle I also do not much like this solution, it does not
work very well when the strings are of very different sizes.
I’ve not found a solution that is really worth the effort needed,
but I think it would be better to have an approach that is more
along the lines of what PIC does: encode only offsets.
In assembler you can just encode the strings and then have an array
with the offsets to them, but in C that is undefined behaviour…
Where performance doesn’t matter, what you definitely can do
is to just dump one string after the other in an array and then
use a function to scan for the desired index.
The ideal solution from my point of view, would be to have
such an array of all strings together and then an array with
the offsets to each.
With some kind of special preprocessor or code generator that
would be easy, but that makes the code messy.
Doing it in pure C also ends up quite a mess, the below is about as
far as I got, no idea if someone knows some magic to make it actually
bearable…


#include 

#define STRINGS \
X(0, 1, "test1") \
X(1, 2, "test2") \
X(2, 3, "testteststeste2") \

#define X(n, m, x) x "\0"
static const char stringdata[] = STRINGS ;
#undef X

#define X(n, m, x) static const int strpos##m = strpos##n + sizeof(x);
static const int strpos0 = 0;
STRINGS
#undef X
int main()
{
printf("0: %s\n", stringdata + strpos0);
printf("1: %s\n", stringdata + strpos1);
printf("2: %s\n", stringdata + strpos2);
}

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] Add support for "omp simd" pragma.

[Reimar Döffinger]

> 
> On 10 Jan 2021, at 19:55, Lynne  wrote:
> 
> Jan 10, 2021, 17:43 by reimar.doeffin...@gmx.de:
> 
>> From: Reimar Döffinger 
>> 
>> real0m15.040s
>> user0m18.874s (80.7% of original)
>> sys 0m0.168s
>> 
> 
> I think I have to disagree.
> The performance gains are marginal,

It’s almost 20%. At least for this combination of
codec and stream a large amount of time is spend in
non-DSP functions, so even hand-written assembler
won’t give you huge gains.


> its definitely something the compiler should
> be able to decide on its own,

So you object to unlikely() macros as well?
It’s really just giving the compiler a hint it should try, though I admit the 
configure part makes it
look otherwise.

> Most of the loops this is added to are trivially SIMDable.

How many hours of effort do you consider “trivial”?
Especially if it’s someone not experienced?
It might be fairly trivial with intrinsics, however
many of your counter-arguments also apply
to intrinsics (and to a degree inline assembly).
That’s btw not just a rhetorical question because
I’m pretty sure I am not going to all the trouble
to port more of the arm 32-bit assembler functions
since it’s a huge PITA, and I was wondering if there
was a point to even have a try with intrinsics...

> Just because no one has
> had the motivation to do SIMD for a pretty unpopular codec doesn't mean we 
> should
> compromise.

If you think of AArch64 specifically, I can
kind of agree.
However I wouldn’t say the word “compromise”
is appropriate when there’s a good chance nothing
better will ever come to exist.
But the real point is not AArch64, that is just
a very convenient test platform.
The point is to raise the minimum bar.
A new architecture, RISC-V for example or something
else should not be stuck at scalar performance
until someone actually gets around to implementing
assembler optimizations.
And just to be clear: I don’t actually care about
HEVC, it just seemed a nice target to do some
experiments.

Best regards,
Reimar
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] libavcodec/hevcdsp: port SIMD idct functions from 32-bit.

[Reimar Döffinger]

> On 12 Jan 2021, at 13:24, Josh Dekker  wrote:
> 
> Hi,
> 
> On 2021-01-08 21:36, reimar.doeffin...@gmx.de wrote:
>> From: Reimar Döffinger 
>> Makes SIMD-optimized 8x8 and 16x16 idcts for 8 and 10 bit depth
>> available on aarch64.
>> For a UHD HDR (10 bit) sample video these were consuming the most time
>> and this optimization reduced overall decode time from 19.4s to 16.4s,
>> approximately 15% speedup.
>> Test sample was the first 300 frames of "LG 4K HDR Demo - New York.ts",
>> running on Apple M1.
>> ---
>> libavcodec/aarch64/Makefile   |   2 +
>> libavcodec/aarch64/hevcdsp_idct_neon.S| 426 ++
>> libavcodec/aarch64/hevcdsp_init_aarch64.c |  45 +++
>> libavcodec/hevcdsp.c  |   2 +
>> libavcodec/hevcdsp.h  |   1 +
>> 5 files changed, 476 insertions(+)
>> create mode 100644 libavcodec/aarch64/hevcdsp_idct_neon.S
>> create mode 100644 libavcodec/aarch64/hevcdsp_init_aarch64.c
>> [...]
> 
> ASlibavcodec/aarch64/hevcdsp_idct_neon.o
> libavcodec/aarch64/hevcdsp_idct_neon.S: Assembler messages:
> libavcodec/aarch64/hevcdsp_idct_neon.S:418: Error: operand mismatch -- `mov 
> v29.4S,v28.4S'

Yes, I noticed that a few days ago, I sent the fixed version now.
I had only tested on Apple assembler, assuming it would be the same.
Really stupid behaviour by the GNU one, as if the type mattered for a mov 
instruction, needlessly complicates macros.

>> Thanks for porting this, I was in the process of writing HEVC
> assembly (see my set on the ML) and would be interested to rebase this on top 
> of that set.

Sorry, I had not seen that as I’ve only recently started reading the list 
(well, only my threads to be honest).
Hope I’ve not duplicated/complicated any of your
work, I was mostly just interested in learning
something new, otherwise I would have checked first
for related work.

Thanks for the interest,
Reimar
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] Project orientation

[Reimar Döffinger]

On Sun, Jul 05, 2020 at 10:50:14PM +0200, Michael Niedermayer wrote:
> To Achieve this, we could try to
> * attract more developers doing reviews, i have generally suggested
>   contributors to help review other peoples patches. Maybe i should
>   take a step back and ask developers to ask developers to do this
>   instead. It is a way out of this problem
> * make people have a burning desire to review patches. I understand
>   this would work very well but iam not sure how to achieve this
> * pay developers to do reviews, i think we do not yet have the funds
>   for this as reviews take alot of time and thus this would not be
>   cheap

Maybe there's not enough people like me to matter, but for me it's
that I don't have time to read the whole mailing list to find the
patches I might be able to contribute to with a review.
I also add the caveat I wouldn't be able to do a super-deep technical
review, but there seem to be enough integer overflow issues (and
I mean the we-all-agree-it's-a-bug type) and other bugs getting
through that anyone familiar with reviewing code would be able
to contribute.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] Project orientation

[Reimar Döffinger]

On Sun, Jul 05, 2020 at 06:18:20PM +0100, Kieran Kunhya wrote:
> On Sun, Jul 5, 2020 at 6:01 PM Kieran Kunhya  wrote:
> >
> > Going back to the original point in hand.
> > Many patches aren't getting reviewed and pushed any more.
> >
> > In part this is because in 2020 whether we like it or not mailing
> > lists are not the way to do Git based development.
> > The kernel is the exception to the rule, as Linus says it has a whole
> > load of grey-bearded system maintainers who are paid full time to work
> > on it.
> >
> > For new contributors git send-email is annoying. For people wanting to
> > push, the .mbox format is annoying, Gmail doesn't support it any more.
> > And you can't get new contributors to start using CLI based email
> > clients or run their own mail server, that's not going to happen.
> >
> > A solution like Gitlab is the only way forward. It has worked well for
> > dav1d, it can run regression tests on all platforms for all commits:
> > https://code.videolan.org/videolan/dav1d
> >
> > Merges are done with one push of a button. Yes, the branch sprawl is
> > not great but it's better than now.
> > It has inline patch reviews which are nice.
> >
> > Whether we like it or not web interfaces are the way 95% of the world
> > does Git and we have to move with the times.
>
> Not my intention to top post but gmail hides quoted text.
> Forgot to add that git send-email is quite complex to setup now
> without your own mail server.
> This also restricts our ability to add new developers.

Is it? It works easily for me just using msmtp or a similar
sendmail implementation that speaks SMTP.
No need for a mail server.
If you think it's an issue, maybe it needs to be documented?
That said, email lists are bad for quick drive-by patches
and there is far too much on this list.
Then again, github/gitlab aren't good for reviews either,
and plain atrocious for community and discussion, IMHO.

Best regards,
Reimar
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH] dnn_backend_native: Add overflow check for length calculation.

[Reimar Döffinger]

We should not silently allocate an incorrect sized buffer.
Fixes trac issue #8718.
TODO1: calculate_operand_dims_count is almost identical code,
should they be merged and its usages check for overflow?
TODO2: the -1 return value seems questionable to me, but is
aligned with the return value used for malloc failure.
Probably both ought to be changed to AVERROR(ENOMEM).

Signed-off-by: Reimar Döffinger 
---
 libavfilter/dnn/dnn_backend_native.c   | 10 +-
 libavfilter/dnn/dnn_backend_native.h   |  2 ++
 libavfilter/dnn/dnn_backend_native_layer_conv2d.c  |  2 ++
 libavfilter/dnn/dnn_backend_native_layer_depth2space.c |  2 ++
 libavfilter/dnn/dnn_backend_native_layer_mathbinary.c  |  2 ++
 libavfilter/dnn/dnn_backend_native_layer_mathunary.c   |  2 ++
 libavfilter/dnn/dnn_backend_native_layer_maximum.c |  2 ++
 libavfilter/dnn/dnn_backend_native_layer_pad.c |  2 ++
 8 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/libavfilter/dnn/dnn_backend_native.c 
b/libavfilter/dnn/dnn_backend_native.c
index 35236fc66f..a685efb092 100644
--- a/libavfilter/dnn/dnn_backend_native.c
+++ b/libavfilter/dnn/dnn_backend_native.c
@@ -79,6 +79,8 @@ static DNNReturnType set_input_output_native(void *model, 
DNNData *input, const

 av_freep(>data);
 oprd->length = calculate_operand_data_length(oprd);
+if (oprd->length <= 0)
+return DNN_ERROR;
 oprd->data = av_malloc(oprd->length);
 if (!oprd->data)
 return DNN_ERROR;
@@ -295,7 +297,13 @@ int32_t calculate_operand_dims_count(const DnnOperand 
*oprd)
 int32_t calculate_operand_data_length(const DnnOperand* oprd)
 {
 // currently, we just support DNN_FLOAT
-return oprd->dims[0] * oprd->dims[1] * oprd->dims[2] * oprd->dims[3] * 
sizeof(float);
+uint64_t len = sizeof(float);
+for (int i = 0; i < 4; i++) {
+len *= oprd->dims[i];
+if (len > INT32_MAX)
+return 0;
+}
+return len;
 }

 void ff_dnn_free_model_native(DNNModel **model)
diff --git a/libavfilter/dnn/dnn_backend_native.h 
b/libavfilter/dnn/dnn_backend_native.h
index bec63be450..62191ffe88 100644
--- a/libavfilter/dnn/dnn_backend_native.h
+++ b/libavfilter/dnn/dnn_backend_native.h
@@ -120,6 +120,8 @@ DNNReturnType ff_dnn_execute_model_native(const DNNModel 
*model, DNNData *output

 void ff_dnn_free_model_native(DNNModel **model);

+// NOTE: User must check for error (return value <= 0) to handle
+// case like integer overflow.
 int32_t calculate_operand_data_length(const DnnOperand *oprd);
 int32_t calculate_operand_dims_count(const DnnOperand *oprd);
 #endif
diff --git a/libavfilter/dnn/dnn_backend_native_layer_conv2d.c 
b/libavfilter/dnn/dnn_backend_native_layer_conv2d.c
index c05bb5eca9..a2202e4073 100644
--- a/libavfilter/dnn/dnn_backend_native_layer_conv2d.c
+++ b/libavfilter/dnn/dnn_backend_native_layer_conv2d.c
@@ -113,6 +113,8 @@ int dnn_execute_layer_conv2d(DnnOperand *operands, const 
int32_t *input_operand_
 output_operand->dims[3] = conv_params->output_num;
 output_operand->data_type = operands[input_operand_index].data_type;
 output_operand->length = calculate_operand_data_length(output_operand);
+if (output_operand->length <= 0)
+return -1;
 output_operand->data = av_realloc(output_operand->data, 
output_operand->length);
 if (!output_operand->data)
 return -1;
diff --git a/libavfilter/dnn/dnn_backend_native_layer_depth2space.c 
b/libavfilter/dnn/dnn_backend_native_layer_depth2space.c
index 324871ceca..2c8bddf23d 100644
--- a/libavfilter/dnn/dnn_backend_native_layer_depth2space.c
+++ b/libavfilter/dnn/dnn_backend_native_layer_depth2space.c
@@ -75,6 +75,8 @@ int dnn_execute_layer_depth2space(DnnOperand *operands, const 
int32_t *input_ope
 output_operand->dims[3] = new_channels;
 output_operand->data_type = operands[input_operand_index].data_type;
 output_operand->length = calculate_operand_data_length(output_operand);
+if (output_operand->length <= 0)
+return -1;
 output_operand->data = av_realloc(output_operand->data, 
output_operand->length);
 if (!output_operand->data)
 return -1;
diff --git a/libavfilter/dnn/dnn_backend_native_layer_mathbinary.c 
b/libavfilter/dnn/dnn_backend_native_layer_mathbinary.c
index b239a20058..dd42c329a9 100644
--- a/libavfilter/dnn/dnn_backend_native_layer_mathbinary.c
+++ b/libavfilter/dnn/dnn_backend_native_layer_mathbinary.c
@@ -91,6 +91,8 @@ int dnn_execute_layer_math_binary(DnnOperand *operands, const 
int32_t *input_ope

 output->data_type = input->data_type;
 output->length = calculate_operand_data_length(output);
+if (output->length <= 0)
+return DNN_ERROR;
 output->data = av_realloc(output->data, output->length);
 if (!output->data)
 return DNN_ERROR;
diff --git a/libavfilter/dnn

[FFmpeg-devel] [PATCH] dnn_backend_native: Add overflow check for length calculation.

[Reimar Döffinger]

We should not silently allocate an incorrect sized buffer.
Fixes trac issue #8718.
TODO1: calculate_operand_dims_count is almost identical code,
should they be merged and its usages check for overflow?
TODO2: the -1 return value seems questionable to me, but is
aligned with the return value used for malloc failure.
Probably both ought to be changed to AVERROR(ENOMEM).

Signed-off-by: Reimar Döffinger 
---
 libavfilter/dnn/dnn_backend_native.c   | 10 +-
 libavfilter/dnn/dnn_backend_native.h   |  2 ++
 libavfilter/dnn/dnn_backend_native_layer_conv2d.c  |  2 ++
 libavfilter/dnn/dnn_backend_native_layer_depth2space.c |  2 ++
 libavfilter/dnn/dnn_backend_native_layer_mathbinary.c  |  2 ++
 libavfilter/dnn/dnn_backend_native_layer_mathunary.c   |  2 ++
 libavfilter/dnn/dnn_backend_native_layer_maximum.c |  2 ++
 libavfilter/dnn/dnn_backend_native_layer_pad.c |  2 ++
 8 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/libavfilter/dnn/dnn_backend_native.c 
b/libavfilter/dnn/dnn_backend_native.c
index 35236fc66f..a685efb092 100644
--- a/libavfilter/dnn/dnn_backend_native.c
+++ b/libavfilter/dnn/dnn_backend_native.c
@@ -79,6 +79,8 @@ static DNNReturnType set_input_output_native(void *model, 
DNNData *input, const

 av_freep(>data);
 oprd->length = calculate_operand_data_length(oprd);
+if (oprd->length <= 0)
+return DNN_ERROR;
 oprd->data = av_malloc(oprd->length);
 if (!oprd->data)
 return DNN_ERROR;
@@ -295,7 +297,13 @@ int32_t calculate_operand_dims_count(const DnnOperand 
*oprd)
 int32_t calculate_operand_data_length(const DnnOperand* oprd)
 {
 // currently, we just support DNN_FLOAT
-return oprd->dims[0] * oprd->dims[1] * oprd->dims[2] * oprd->dims[3] * 
sizeof(float);
+uint64_t len = sizeof(float);
+for (int i = 0; i < 4; i++) {
+len *= oprd->dims[i];
+if (len > INT32_MAX)
+return 0;
+}
+return len;
 }

 void ff_dnn_free_model_native(DNNModel **model)
diff --git a/libavfilter/dnn/dnn_backend_native.h 
b/libavfilter/dnn/dnn_backend_native.h
index bec63be450..62191ffe88 100644
--- a/libavfilter/dnn/dnn_backend_native.h
+++ b/libavfilter/dnn/dnn_backend_native.h
@@ -120,6 +120,8 @@ DNNReturnType ff_dnn_execute_model_native(const DNNModel 
*model, DNNData *output

 void ff_dnn_free_model_native(DNNModel **model);

+// NOTE: User must check for error (return value <= 0) to handle
+// case like integer overflow.
 int32_t calculate_operand_data_length(const DnnOperand *oprd);
 int32_t calculate_operand_dims_count(const DnnOperand *oprd);
 #endif
diff --git a/libavfilter/dnn/dnn_backend_native_layer_conv2d.c 
b/libavfilter/dnn/dnn_backend_native_layer_conv2d.c
index c05bb5eca9..a2202e4073 100644
--- a/libavfilter/dnn/dnn_backend_native_layer_conv2d.c
+++ b/libavfilter/dnn/dnn_backend_native_layer_conv2d.c
@@ -113,6 +113,8 @@ int dnn_execute_layer_conv2d(DnnOperand *operands, const 
int32_t *input_operand_
 output_operand->dims[3] = conv_params->output_num;
 output_operand->data_type = operands[input_operand_index].data_type;
 output_operand->length = calculate_operand_data_length(output_operand);
+if (output_operand->length <= 0)
+return -1;
 output_operand->data = av_realloc(output_operand->data, 
output_operand->length);
 if (!output_operand->data)
 return -1;
diff --git a/libavfilter/dnn/dnn_backend_native_layer_depth2space.c 
b/libavfilter/dnn/dnn_backend_native_layer_depth2space.c
index 324871ceca..2c8bddf23d 100644
--- a/libavfilter/dnn/dnn_backend_native_layer_depth2space.c
+++ b/libavfilter/dnn/dnn_backend_native_layer_depth2space.c
@@ -75,6 +75,8 @@ int dnn_execute_layer_depth2space(DnnOperand *operands, const 
int32_t *input_ope
 output_operand->dims[3] = new_channels;
 output_operand->data_type = operands[input_operand_index].data_type;
 output_operand->length = calculate_operand_data_length(output_operand);
+if (output_operand->length <= 0)
+return -1;
 output_operand->data = av_realloc(output_operand->data, 
output_operand->length);
 if (!output_operand->data)
 return -1;
diff --git a/libavfilter/dnn/dnn_backend_native_layer_mathbinary.c 
b/libavfilter/dnn/dnn_backend_native_layer_mathbinary.c
index b239a20058..bb30ccf154 100644
--- a/libavfilter/dnn/dnn_backend_native_layer_mathbinary.c
+++ b/libavfilter/dnn/dnn_backend_native_layer_mathbinary.c
@@ -91,6 +91,8 @@ int dnn_execute_layer_math_binary(DnnOperand *operands, const 
int32_t *input_ope

 output->data_type = input->data_type;
 output->length = calculate_operand_data_length(output);
+if (output->length)
+return DNN_ERROR;
 output->data = av_realloc(output->data, output->length);
 if (!output->data)
 return DNN_ERROR;
diff --git a/libavfilter/dnn/dn

Re: [FFmpeg-devel] [PATCH]lavf/r3d: Check avio_read() return value

[Reimar Döffinger]

On Thu, Aug 22, 2019 at 12:06:03PM +0200, Carl Eugen Hoyos wrote:
> Hi!
>
> Attached patch fixes usage of uninitialized data reading broken r3d files.
>
> Please review, Carl Eugen

> From efce9940b890b9cf5a9e7400b05be10f6906fbb1 Mon Sep 17 00:00:00 2001
> From: Carl Eugen Hoyos 
> Date: Thu, 22 Aug 2019 12:02:36 +0200
> Subject: [PATCH] lavf/r3d: Check avio_read() return value.
>
> Fixes use of uninitialized data reading broken files.
> Reported-by: Anatoly Trosinenko 
> ---
>  libavformat/r3d.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/libavformat/r3d.c b/libavformat/r3d.c
> index 224bcf780d..919c3c4960 100644
> --- a/libavformat/r3d.c
> +++ b/libavformat/r3d.c
> @@ -98,7 +98,8 @@ static int r3d_read_red1(AVFormatContext *s)
>  r3d->audio_channels = avio_r8(s->pb); // audio channels
>  av_log(s, AV_LOG_TRACE, "audio channels %d\n", tmp);
>
> -avio_read(s->pb, filename, 257);
> +if (avio_read(s->pb, filename, 257) < 0)
> +return AVERROR_INVALIDDATA;
>  filename[sizeof(filename)-1] = 0;

As far as I can tell most code would either return the avio_read return
value or AVERROR(EIO).
Also I think short reads are also an issue and would
cause the same issue?
Maybe it would be sensible to also change the 257 to
sizeof(filename)-1 while at it?
Overall something along the lines (with possibly less stupid names) of

namebytes = sizeof(filename)-1;
ret = avio_read(s->pb, filename, namebytes);
if (ret < namebytes)
return ret < 0 ? ret : AVERROR(EIO);
filename[namebytes] = 0;

The hard failure return here seems appropriate as this logic reads the
very start of the file, so if a read error happens here I
don't think there's any way to recover anything.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [CALL] New FFmpeg developers meeting

[Reimar Döffinger]

On 20.08.2019, at 10:57, Paul B Mahol  wrote:
> Because of current overall toxic situation in FFmpeg, meeting will not be
> held until situation improves considerably.

It's a bit strange to read a such a opposite statement without any reason given 
for the change of mind.
Also while I can see that it can and likely will feel quite uncomfortable, an 
actual meeting usually is one of the more effective ways to actually improve 
the situation.
Though admittedly it doesn't need to be anything "formally" organized.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 3/6] avcodec/pngdec: Optimize has_trns code

[Reimar Döffinger]

On 18.08.2019, at 01:28, Michael Niedermayer  wrote:

> 30M cycles -> 5M cycles

I see nothing wrong with it, but:
You could save reviewers a lot of time if you gave them a hint of what the 
change
contains instead of them having to reverse-engineer.
In this case for example something like
"add inner loop specialisations for 2 bpp and 4 bpp"

> 
> @@ -1363,19 +1364,38 @@ exit_loop:
> unsigned x, y;
> 
> av_assert0(s->bit_depth > 1);
> -

Cosmetic?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [REQUEST] ffmpeg-security subscription

[Reimar Döffinger]

On 15.08.2019, at 13:15, Vittorio Giovara  wrote:
>  if you use ffmpeg in your $dayjob, being notified of security problem
> in ffmpeg, and acting upon it before the fix lands in the tree, may be
> crucial.

I realize I only responded to this specific part only in the context of this 
discussion.
Which might give the wrong impression.
I'd LOVE for someone to come up with documentation and criteria and then create 
and managing a mailing
list of important and trusted USERS (which might overlap with developers, but 
I'd expect it to be more people in admin/deployment positions, or DevOps or 
such), similarly to what the Linux kernel has.
And a guideline for when it would be used.
I would expect anyone handling security issues would make an effort to have 
that list involved as appropriate and be happy to have a way to give a heads-up 
to critical users (correct me if I'm wrong on that).
But it would need one or more volunteers to do the work.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [REQUEST] ffmpeg-security subscription

[Reimar Döffinger]

On 15.08.2019, at 19:38, Paul B Mahol  wrote:

> On Thu, Aug 15, 2019 at 7:20 PM Reimar Döffinger 
> wrote:
> 
>> On 15.08.2019, at 13:15, Vittorio Giovara 
>> wrote:
>>> I think being on the security list may have some professional
>> implications
>>> too: if you use ffmpeg in your $dayjob, being notified of security
>> problem
>>> in ffmpeg, and acting upon it before the fix lands in the tree, may be
>>> crucial. I think Paul is lamenting the fact that being selected for the
>>> security list is extremely arbitrary and there is no process described on
>>> how to joining it.
>> 
>> Sorry, but just any $dayjob I really don't see relevant at all.
>> If there is a huge user of AND major contributor to FFmpeg with vastly
>> higher risk of attack that is hard to mitigate in any other way they might
>> have an argument. I.e. if there is a NEED because it is the only way to
>> protect a significant user/number of users.
>> But it still most likely is a misuse. The security list is about receiving
>> reports and responding to it from our side.
>> Using it to forewarn users would either mean letting a large number of
>> people on it (I hope we agree that is obviously stupid) or disadvantaging >
>> 99% of our users.
>> If someone has concerns in this area and I'm sure there's ways for them to
>> contribute.
>> I still don't see it would need access to the security list though, but it
>> might lead to being invited.
>> 
>> Of course this is just my opinion and I am happy to learn:
>> are there other projects describing such a process?
>> For the Linux kernel I only know about such a thing for the list that is
>> for communicating and aligning with distributions.
>> Something comparable does not currently exist for FFmpeg.
>> 
> 
> So you, as developer are higher valued and more useful than other
> developers?

I have no idea where you get that from anything I said, do you think the bus 
driver is higher valued and more useful than anyone else on the bus because 
they don't let just anyone who wants drive it?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [REQUEST] ffmpeg-security subscription

[Reimar Döffinger]

On 15.08.2019, at 13:15, Vittorio Giovara  wrote:
> I think being on the security list may have some professional implications
> too: if you use ffmpeg in your $dayjob, being notified of security problem
> in ffmpeg, and acting upon it before the fix lands in the tree, may be
> crucial. I think Paul is lamenting the fact that being selected for the
> security list is extremely arbitrary and there is no process described on
> how to joining it.

Sorry, but just any $dayjob I really don't see relevant at all.
If there is a huge user of AND major contributor to FFmpeg with vastly higher 
risk of attack that is hard to mitigate in any other way they might have an 
argument. I.e. if there is a NEED because it is the only way to protect a 
significant user/number of users.
But it still most likely is a misuse. The security list is about receiving 
reports and responding to it from our side.
Using it to forewarn users would either mean letting a large number of people 
on it (I hope we agree that is obviously stupid) or disadvantaging > 99% of our 
users.
If someone has concerns in this area and I'm sure there's ways for them to 
contribute.
I still don't see it would need access to the security list though, but it 
might lead to being invited.

Of course this is just my opinion and I am happy to learn:
are there other projects describing such a process?
For the Linux kernel I only know about such a thing for the list that is for 
communicating and aligning with distributions.
Something comparable does not currently exist for FFmpeg.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [REQUEST] ffmpeg-security subscription

[Reimar Döffinger]

On 14.08.2019, at 11:45, Paul B Mahol  wrote:
> I strongly disagree with you. Why some people have subscription to security
> mailing list and I'm not allowed also?

Long version, explaining to the best of my knowledge and memory:
The people on it are on it because at some point it was considered necessary to 
have them on it.
You have not brought an argument why the project would need you to be on it in 
order to deal with issues in a satisfactory way.
Generally only basic technical skills are needed, enough to discuss the issue 
and potentially hand over to the maintainer. And whoever is involved in the 
releases is generally needed.
Beyond that I would describe it as a PR function (giving a polite and level 
headed response to a security researcher claiming something that is obvious 
nonsense to an FFmpeg developer tends to make things go much smoother), which I 
would have assumed to not be among your aspirations.
It's definitely not about a "right" or a "priviledge" or having "earned" it, 
it's about need.
And when possible a bit of trust (the personal kind, not just the "not 
malicious" kind which is of course an absolute requirement), though that might 
be more relevant for projects like Linux where really bad stuff causing stress 
and possibly conflicts tends to hit. Flame wars is the last thing one needs in 
the middle of dealing with a bad issue.

TL;DR is probably: one doesn't end up on security lists by asking to be on it 
but by persons Y and Z saying "we should/need to have person X on the list".
I am not aware of any such wishes (though admittedly I wouldn't be the one 
contacted about it I guess).
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [REQUEST] ffmpeg-security subscription

[Reimar Döffinger]

On 13.08.2019, at 09:45, Paul B Mahol  wrote:

> On Mon, Aug 12, 2019 at 6:15 PM Michael Niedermayer 
> wrote:
> 
>> Hi Paul
>> 
>> On Mon, Aug 05, 2019 at 11:50:04AM +0200, Paul B Mahol wrote:
>>> Hi,
>>> 
>>> I here hereby request from lead FFmpeg entity to give me subscription to
>>> ffmpeg-security mailing list.
>> 
>> I am not sure who or what a "lead FFmpeg entity" is, But as iam being
>> highlighted
>> on IRC by you in relation to this, and as iam the most active developer on
>> security issues in ffmpeg it would be inpolite from me if i didnt say
>> something.
>> 
> 
> You are the only one working on this.

What is "this"? Michael is handling things coming in very quickly so there 
rarely is any need,
but I believe there are more people around with experience of handling reported 
issues.

>> About ffmpeg-security,
>> Theres currently no need for more manpower to handle security issues. We
>> have
>> a backlog of a few days of fuzzing issues only which is shrinking. And no
>> other
>> issues besides fuzzing issues. (part of the backlog probably was the
>> result
>> of distractions and some longer review cycles on some patches recently)
>> Also all patches are being posted in public so no access is needed for
>> reviews.
>> 
> 
> I strongly disagree. And I haven't asked if you need help.

Wait, is this about the fuzzing?
If so, I really disagree with throwing that in one pot with the handling of
security reports that might come with a polished exploit or even are active in 
the wild
(well, so far we've been lucky on that front to be spared that, but still).
Maybe people disagree, but I see the fuzzing as a development tool primarily,
and as such would probably consider quite different criteria applicable for 
access
compared to the security alias in general.
(which is again a different question from what wishes make sense to accomodate 
purely
from an effort point of view, but there might be a point that I believe Michael 
is the only one
with experience dealing with the fuzzer which is a non-optimal "single point of 
failure")

For non-fuzzing security issues, that usually is done on a pretty much 
need-to-know basis,
often based on who might be unavoidable to involve anyway, or who has access 
anyway.
Thus Michael's reply of not needing help is relevant - without a need the 
default response is likely
to involve people only on a case-by-case basis (generally, maintainers would 
and should be involved if the issue is related to their code).

That is my point of view at least, not sure if distinguishing fuzzing and other 
things is controversial.

Best regards,
Reimar Döffinger
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] avutil: Add Simple loop detector

[Reimar Döffinger]

On 12.08.2019, at 21:53, Michael Niedermayer  wrote:

> On Sun, Aug 11, 2019 at 08:30:51PM +0200, Reimar Döffinger wrote:
>> On 08.08.2019, at 10:36, Michael Niedermayer  wrote:
>> 
>>> This provides an alternative to retry counters.
>>> Useful if there is no reasonable maximum number of iterations and
>>> no ordering that naturally avoids loops.
>> 
>> Going by the old principle of "an API is not tested until it has at least 3 
>> users"
>> might it make sense to delay this until we've found and tested it in a few 
>> use-cases?
>> Depending on how much hurry there is to get the bug-fix in.
> 
>> I assume there is also an actual bug-fix patch somewhere, maybe we should 
>> have that
>> in the same patch series to make it easier to review the actual usage?
> 
> sure will repost this eventually with 3+ bugfixes.
> But wont search for such bugs ATM as ive too many other things to do
> so it might take a bit of time before i do

Of course.
Though on re-considering: if it is added as a purely internal API that we can 
change at any
time and we do not need to think on backwards compatibility (and a comment on 
the file
that we might want to have and review use-cases before making it public) I 
would have
no objections.
I realized only being locked-in compatibility-wise had me worried at this point 
actually.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/3] avformat/avio: add avio_print_n_strings and avio_print

[Reimar Döffinger]

On 11.08.2019, at 21:51, Marton Balint  wrote:

> 
> 
> On Sun, 11 Aug 2019, Reimar Döffinger wrote:
> 
>> On 11.08.2019, at 20:41, Reimar Döffinger  wrote:
>> 
>>> On 05.08.2019, at 23:34, Marton Balint  wrote:
>>>> These functions can be used to print a variable number of strings 
>>>> consecutively
>>>> to the IO context. Unlike av_bprintf, no temporery buffer is necessary.
>>> Hm, is there a use-example patch I missed?
>> 
>> Sorry, I see it now, and also the argument about the macro providing type 
>> checking.
>> Ignore me if you feel the code is fine as-is.
>> 
>>> Also is the #define ugliness worth it compared to just requiring NULL 
>>> termination?
>>> You can use the sentinel function attribute to have the compiler check that 
>>> users do not forget it.
> 
> There is yet another approach which does not count parameters but uses a NULL 
> sentinel, and is more simple:
> 
> void avio_print_string_array(AVIOContext *s, const char *strings[])
> {
>for(; strings[0]; strings++)
>avio_write(s, (const unsigned char *)strings[0], strlen(strings[0]));
> }

Nitpick: I find using [0] confusing when used on a loop variable and would 
consider *strings better

> #define avio_print(s, ...) \
>avio_print_string_array(s, (const char*[]){__VA_ARGS__, NULL})
> 
> This might also has the benefit that __VA_ARGS__ is referenced only once, 
> because for the old version some compilers (msvc) was not optimizing away the 
> additional local variable and their string references which were used in the 
> sizeof magic.
> 
> I can change the patch to use this version if people prefer.

It also avoids potential multiple-evaluation issues, so it does seem more 
robust to me.
But I admit my macro-foo is rather weak due to too much C++ use in the last 
years...
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2 2/2] swscale: Fix AltiVec/VSX build with recent GCC

[Reimar Döffinger]

On 11.08.2019, at 21:24, Reimar Döffinger  wrote:

> On 07.08.2019, at 19:39, Daniel Kolesa  wrote:
> 
>> The argument to vec_splat_u16 must be a literal. By making the
>> function always inline and marking the arguments const, gcc can
>> turn those into literals, and avoid build errors like:
> 
> Why marking the arguments const?
> If it depends on that it sounds like this might be really unreliable and just 
> work or not work with random compiler versions.
> It would also be nice to know if/what the impact on code size or performance 
> would be of always inline.
> An alternative, uglier but likely more reliable option would be to pass the 
> vswap/vshift vectors as arguments and have a macro that generates them (admit 
> the multiple-evaluation risks though)
> e.g.
> #define yuv2plane1_16_vsx(s, d, w, b, o) yuv2plane1_16_vsx(s, d, w, b, o, 
> vec_splat_u16(b ? 8 : 0))

I also realise now that the vec_splat_u16 is just an optimization.
So maybe the first step would to be to check if there is actually a relevant 
performance advantage or if it wouldn't be simplest to just use the generic 
initialization code.

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2 1/2] swscale: Replace illegal vector keyword usage in altivec code

[Reimar Döffinger]

On 07.08.2019, at 19:39, Daniel Kolesa  wrote:

> While this technically compiles in current ffmpeg, this is only
> because ffmpeg is compiled in strict ISO C mode, which disables
> the builtin 'vector' keyword for AltiVec/VSX. Instead this gets
> replaced with a macro inside altivec.h, which defines vector to
> be actually __vector, which accepts random types.
> 
> Normally, the vector keyword should be used only with plain
> scalar non-typedef types, such as unsigned int. But we have the
> vec_(s|u)(8|16|32) macros, which can be used in a portable manner,
> in util_altivec.h in libavutil.
> 
> This is also consistent with other AltiVec/VSX code elsewhere in
> the tree.

I see the consistency argument, but otherwise it really doesn't sound 
convincing.
I don't think the intend to support compiling with -std=gnu11 either way, so 
not sure we should care about that.
And not allowing typedef'd type seems utterly silly and against any expected C 
behaviour that for simple maintenance reasons I don't think we want this 
long-term.
As far as I remember switching to __vector everywhere is not really an option 
either though because some compilers do not accept that?
I don't strictly object to the change, it just doesn't really feel like a truly 
good path forward long-term.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2 2/2] swscale: Fix AltiVec/VSX build with recent GCC

[Reimar Döffinger]

On 07.08.2019, at 19:39, Daniel Kolesa  wrote:

> The argument to vec_splat_u16 must be a literal. By making the
> function always inline and marking the arguments const, gcc can
> turn those into literals, and avoid build errors like:

Why marking the arguments const?
If it depends on that it sounds like this might be really unreliable and just 
work or not work with random compiler versions.
It would also be nice to know if/what the impact on code size or performance 
would be of always inline.
An alternative, uglier but likely more reliable option would be to pass the 
vswap/vshift vectors as arguments and have a macro that generates them (admit 
the multiple-evaluation risks though)
e.g.
#define yuv2plane1_16_vsx(s, d, w, b, o) yuv2plane1_16_vsx(s, d, w, b, o, 
vec_splat_u16(b ? 8 : 0))

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/3] avformat/avio: add avio_print_n_strings and avio_print

[Reimar Döffinger]

On 11.08.2019, at 20:41, Reimar Döffinger  wrote:

> On 05.08.2019, at 23:34, Marton Balint  wrote:
> 
>> These functions can be used to print a variable number of strings 
>> consecutively
>> to the IO context. Unlike av_bprintf, no temporery buffer is necessary.
> 
> Hm, is there a use-example patch I missed?

Sorry, I see it now, and also the argument about the macro providing type 
checking.
Ignore me if you feel the code is fine as-is.

> Also is the #define ugliness worth it compared to just requiring NULL 
> termination?
> You can use the sentinel function attribute to have the compiler check that 
> users do not forget it.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/3] avformat/avio: add avio_print_n_strings and avio_print

[Reimar Döffinger]

On 05.08.2019, at 23:34, Marton Balint  wrote:

> These functions can be used to print a variable number of strings 
> consecutively
> to the IO context. Unlike av_bprintf, no temporery buffer is necessary.

Hm, is there a use-example patch I missed?
Also is the #define ugliness worth it compared to just requiring NULL 
termination?
You can use the sentinel function attribute to have the compiler check that 
users do not forget it.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] avutil: Add Simple loop detector

[Reimar Döffinger]

On 08.08.2019, at 10:36, Michael Niedermayer  wrote:

> This provides an alternative to retry counters.
> Useful if there is no reasonable maximum number of iterations and
> no ordering that naturally avoids loops.

Going by the old principle of "an API is not tested until it has at least 3 
users"
might it make sense to delay this until we've found and tested it in a few 
use-cases?
Depending on how much hurry there is to get the bug-fix in.
I assume there is also an actual bug-fix patch somewhere, maybe we should have 
that
in the same patch series to make it easier to review the actual usage?

> diff --git a/doc/APIchanges b/doc/APIchanges
> index 6603a8229e..eee4c30ec5 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,9 @@ libavutil: 2017-10-21
> 
> API changes, most recent first:
> 
> +2019-XX-XX - XX - lavu 56.XX.XXX - loop_detector.h
> +  Add loop_detector.h, av_is_loop(), AVSimpleLoopDetector

Does is mean it is a public/installed header?
I'd prefer to keep new APIs internal until they are really proven in use if 
there is no immediate pressure.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v5] avutil/mips: Avoid instruction exception caused by gssqc1/gslqc1.

[Reimar Döffinger]

On Wed, Jul 31, 2019 at 09:30:01AM +0800, Shiyou Yin wrote:
> Ensure the address accesed by gssqc1/gslqc1 are 16-byte aligned.

Pushed, thanks.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v5] avutil/mips: Avoid instruction exception caused by gssqc1/gslqc1.

[Reimar Döffinger]

Looks good to me

On 31.07.2019, at 03:30, Shiyou Yin  wrote:

> Ensure the address accesed by gssqc1/gslqc1 are 16-byte aligned.
> ---
> libavcodec/mips/simple_idct_mmi.c | 2 +-
> libavutil/mips/mmiutils.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/mips/simple_idct_mmi.c 
> b/libavcodec/mips/simple_idct_mmi.c
> index 7f4bb74..73d797f 100644
> --- a/libavcodec/mips/simple_idct_mmi.c
> +++ b/libavcodec/mips/simple_idct_mmi.c
> @@ -39,7 +39,7 @@
> #define COL_SHIFT 20
> #define DC_SHIFT 3
> 
> -DECLARE_ALIGNED(8, const int16_t, W_arr)[46] = {
> +DECLARE_ALIGNED(16, const int16_t, W_arr)[46] = {
> W4,  W2,  W4,  W6,
> W1,  W3,  W5,  W7,
> W4,  W6, -W4, -W2,
> diff --git a/libavutil/mips/mmiutils.h b/libavutil/mips/mmiutils.h
> index 05f6b31..8f692e8 100644
> --- a/libavutil/mips/mmiutils.h
> +++ b/libavutil/mips/mmiutils.h
> @@ -205,7 +205,7 @@
>  * backup register
>  */
> #define BACKUP_REG \
> -  double temp_backup_reg[8];\
> +  LOCAL_ALIGNED_16(double, temp_backup_reg, [8]);   \
>   if (_MIPS_SIM == _ABI64)  \
> __asm__ volatile (  \
>   "gssqc1   $f25,  $f24,   0x00(%[temp])  \n\t" \
> -- 
> 2.1.0
> 
> 
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avcodec/arm/sbcenc: save callee preserved vfp registers

[Reimar Döffinger]

Seems sensible to me, though extra points if you or someone has numbers on 
performance impact.
To know whether it would be worthwhile to check if it can be optimized...

On 28.07.2019, at 23:46, James Cowgill  wrote:

> When compiling FFmpeg with GCC-9, some very random segfaults were
> observed in code which had previously called down into the SBC encoder
> NEON assembly routines. This was caused by these functions clobbering
> some of the vfp callee saved registers (d8 - d15 aka q4 - q7). GCC was
> using these registers to save local variables, but after these
> functions returned, they would contain garbage.
> 
> Fix by saving the relevant registers on the stack in the affected
> functions.
> 
> Signed-off-by: James Cowgill 
> ---
> libavcodec/arm/sbcdsp_neon.S | 6 ++
> 1 file changed, 6 insertions(+)
> 
> diff --git a/libavcodec/arm/sbcdsp_neon.S b/libavcodec/arm/sbcdsp_neon.S
> index d83d21d202..aa03800096 100644
> --- a/libavcodec/arm/sbcdsp_neon.S
> +++ b/libavcodec/arm/sbcdsp_neon.S
> @@ -38,6 +38,8 @@ function ff_sbc_analyze_4_neon, export=1
> /* TODO: merge even and odd cases (or even merge all four calls to 
> this
>  * function) in order to have only aligned reads from 'in' array
>  * and reduce number of load instructions */
> +vpush   {d8-d11}
> +
> vld1.16 {d4, d5}, [r0, :64]!
> vld1.16 {d8, d9}, [r2, :128]!
> 
> @@ -84,6 +86,7 @@ function ff_sbc_analyze_4_neon, export=1
> 
> vst1.32 {d0, d1}, [r1, :128]
> 
> +vpop{d8-d11}
> bx  lr
> endfunc
> 
> @@ -91,6 +94,8 @@ function ff_sbc_analyze_8_neon, export=1
> /* TODO: merge even and odd cases (or even merge all four calls to 
> this
>  * function) in order to have only aligned reads from 'in' array
>  * and reduce number of load instructions */
> +vpush   {d8-d15}
> +
> vld1.16 {d4, d5}, [r0, :64]!
> vld1.16 {d8, d9}, [r2, :128]!
> 
> @@ -188,6 +193,7 @@ function ff_sbc_analyze_8_neon, export=1
> 
> vst1.32 {d0, d1, d2, d3}, [r1, :128]
> 
> +vpop{d8-d15}
> bx  lr
> endfunc
> 
> -- 
> 2.22.0
> 
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v3] avutil/mips: Avoid instruction exception caused by gssqc1/gslqc1.

[Reimar Döffinger]

On 29.07.2019, at 11:54, "Shiyou Yin"  wrote:
>> 
> DECLARE_ALIGNED is defined in ' libavutil/mem.h ' and related to compiler. No 
> matter mips or x86,
> it's definition is ' #define DECLARE_ALIGNED(n,t,v)  t __attribute__ 
> ((aligned (n))) v' when build 
> with gcc or clang (Specific implementation within the compiler is not 
> considered here.).
> In libavcodec/x86, DECLARE_ALIGNED is used to define 8/16/32-byte aligned 
> variable too.

The aligned attribute does not work reliably with stack variables in some cases.
Compare with other code, I think you need to use LOCAL_ALIGNED_16 for the stack 
variable.
Yes, it might work in your test even with DECLARE_ALIGNED, but it might not be 
robust.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v7 2/2] lavc/tiff: Decode embedded JPEGs in DNG images

[Reimar Döffinger]

On 28.07.2019, at 16:45, Paul B Mahol  wrote:

> On Sun, Jul 28, 2019 at 4:39 PM Reimar Döffinger 
> wrote:
> 
>> On 28.07.2019, at 08:55, Paul B Mahol  wrote:
>> 
>>>> 
>>>> Just provide as metadata and leave to e.g. libavfilter.
>>>> 
>>> 
>>> That does not follow DNG specification.
>>> I really do not have time to comment on other irrelevant stuff you
>> pointed
>>> in your review.
>> 
>> If you had just told me that I should back off in a polite manner instead
>> of this dismissive-insulting
>> way it would have taken me much less effort to acknowledge your point.
>> And over time you could save a lot of time thanks to not having to complain
>> about people being biased against you.
> 
> 
> Sorry if you feel attacked, but I simply do not like it.

That is fine to tell me. I'll at least make an effort. Just the wording 
discourages from doing so.

> Instead of complaining you could send patch one or two.

(Assuming you mean the review by "complaining")
Sorry, it was just meant to be a review, not complaining, but I know I tend to 
write in a way that is too much like complaining.
There are quite a few patches that go unreviewed because (I assume) there are 
not enough reviewers as usual, so I've rather been reviewing than developing.
Plus I have no personal itch to scratch to send a patch for currently.
But if more people think there are more useful ways to contribute I'll 
certainly listen.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v7 2/2] lavc/tiff: Decode embedded JPEGs in DNG images

[Reimar Döffinger]

On 28.07.2019, at 08:55, Paul B Mahol  wrote:

>> 
>> Just provide as metadata and leave to e.g. libavfilter.
>> 
> 
> That does not follow DNG specification.
> I really do not have time to comment on other irrelevant stuff you pointed
> in your review.

If you had just told me that I should back off in a polite manner instead of 
this dismissive-insulting
way it would have taken me much less effort to acknowledge your point.
And over time you could save a lot of time thanks to not having to complain
about people being biased against you.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v7 2/2] lavc/tiff: Decode embedded JPEGs in DNG images

[Reimar Döffinger]

On 28.07.2019, at 10:40, Nick Renieris  wrote:

> Actually, I checked a more accurate version of my loop, and GCC
> optimizes away the LUT check anyway:
> https://godbolt.org/z/G1e1R4
> As you can see it's smart enough to create 2 versions of my functions
> (started at L3 with a lookup and L7 without it) and it does the check
> outside.
> 
> There's no guarantee this is happening with the actual version of
> course (it could be slower, or even faster if it also optimizes it
> through dng_blit).
> I could check the actual disasm in FFmpeg, but I don't think it's
> worth it at this point (my mentor agrees).

Sorry, I did not know you already had discussions with an FFmpeg developer 
about the design.
I tend to review in a way that I just comment on anything I feel is not optimal.
I understand that much of it might not be reasonable to do differently in the 
end,
but feel that it is often enough to make it worth discussing things.
But I realize it can come across the wrong way, so sorry if I gave you a bad 
review experience,
and thanks for considering my comments.

Best regards,
Reimar
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2] avcodec/mips: [loongson] refine process of setting block as 0 in h264dsp_mmi.

[Reimar Döffinger]

I have no MIPS experience, but for what little it is worth thus: it looks fine 
to me.

On 28.07.2019, at 06:42, Shiyou Yin  wrote:

> In function ff_h264_add_pixels4_8_mmi, there is no need to reset '%[ftmp0]'
> to 0, because it's value has never changed since the start of the asm block.
> This patch remove the redundant 'xor' and set src to zero once it was loaded.
> 
> In function ff_h264_idct_add_8_mmi, 'block' is seted to zero twice.
> This patch removed the first setting zero operation and move the second one
> after the load operation of block.
> 
> In function ff_h264_idct8_add_8_mmi, 'block' is seted to zero twice too.
> This patch just removed the second setting zero operation.
> 
> This patch mainly simplifies the implementation of functions above,
> the effect on the performance of whole h264 decoding process is not obvious.
> According to the perf data, proportion of ff_h264_idct_add_8_mmi decreased 
> from
> 0.29% to 0.26% and ff_h264_idct8_add_8_mmi decreased from 0.62% to 0.59% when 
> decoding
> H264 format on loongson 3A3000(For reference only , not very stable.).
> ---
> libavcodec/mips/h264dsp_mmi.c | 44 +--
> 1 file changed, 13 insertions(+), 31 deletions(-)
> 
> diff --git a/libavcodec/mips/h264dsp_mmi.c b/libavcodec/mips/h264dsp_mmi.c
> index ac65a20..0459711 100644
> --- a/libavcodec/mips/h264dsp_mmi.c
> +++ b/libavcodec/mips/h264dsp_mmi.c
> @@ -38,6 +38,9 @@ void ff_h264_add_pixels4_8_mmi(uint8_t *dst, int16_t *src, 
> int stride)
> MMI_LDC1(%[ftmp2], %[src], 0x08)
> MMI_LDC1(%[ftmp3], %[src], 0x10)
> MMI_LDC1(%[ftmp4], %[src], 0x18)
> +/* memset(src, 0, 32); */
> +"gssqc1 %[ftmp0],   %[ftmp0],   0x00(%[src])\n\t"
> +"gssqc1 %[ftmp0],   %[ftmp0],   0x10(%[src])\n\t"
> MMI_ULWC1(%[ftmp5], %[dst0], 0x00)
> MMI_ULWC1(%[ftmp6], %[dst1], 0x00)
> MMI_ULWC1(%[ftmp7], %[dst2], 0x00)
> @@ -58,11 +61,6 @@ void ff_h264_add_pixels4_8_mmi(uint8_t *dst, int16_t *src, 
> int stride)
> MMI_SWC1(%[ftmp2], %[dst1], 0x00)
> MMI_SWC1(%[ftmp3], %[dst2], 0x00)
> MMI_SWC1(%[ftmp4], %[dst3], 0x00)
> -
> -/* memset(src, 0, 32); */
> -"xor%[ftmp0],   %[ftmp0],   %[ftmp0]\n\t"
> -"gssqc1 %[ftmp0],   %[ftmp0],   0x00(%[src])\n\t"
> -"gssqc1 %[ftmp0],   %[ftmp0],   0x10(%[src])\n\t"
> : [ftmp0]"="(ftmp[0]),[ftmp1]"="(ftmp[1]),
>   [ftmp2]"="(ftmp[2]),[ftmp3]"="(ftmp[3]),
>   [ftmp4]"="(ftmp[4]),[ftmp5]"="(ftmp[5]),
> @@ -85,15 +83,19 @@ void ff_h264_idct_add_8_mmi(uint8_t *dst, int16_t *block, 
> int stride)
> DECLARE_VAR_ADDRT;
> 
> __asm__ volatile (
> -"dli%[tmp0],0x01\n\t"
> MMI_LDC1(%[ftmp0], %[block], 0x00)
> -"mtc1   %[tmp0],%[ftmp8]\n\t"
> MMI_LDC1(%[ftmp1], %[block], 0x08)
> -"dli%[tmp0],0x06\n\t"
> MMI_LDC1(%[ftmp2], %[block], 0x10)
> +MMI_LDC1(%[ftmp3], %[block], 0x18)
> +/* memset(block, 0, 32) */
> +"xor%[ftmp4],   %[ftmp4],   %[ftmp4]\n\t"
> +"gssqc1 %[ftmp4],   %[ftmp4],   0x00(%[block])  \n\t"
> +"gssqc1 %[ftmp4],   %[ftmp4],   0x10(%[block])  \n\t"
> +"dli%[tmp0],0x01\n\t"
> +"mtc1   %[tmp0],%[ftmp8]\n\t"
> +"dli%[tmp0],0x06\n\t"
> "mtc1   %[tmp0],%[ftmp9]\n\t"
> "psrah  %[ftmp4],   %[ftmp1],   %[ftmp8]\n\t"
> -MMI_LDC1(%[ftmp3], %[block], 0x18)
> "psrah  %[ftmp5],   %[ftmp3],   %[ftmp8]\n\t"
> "psubh  %[ftmp4],   %[ftmp4],   %[ftmp3]\n\t"
> "paddh  %[ftmp5],   %[ftmp5],   %[ftmp1]\n\t"
> @@ -121,15 +123,11 @@ void ff_h264_idct_add_8_mmi(uint8_t *dst, int16_t 
> *block, int stride)
> "paddh  %[ftmp10],  %[ftmp3],   %[ftmp1]\n\t"
> "psubh  %[ftmp1],   %[ftmp1],   %[ftmp3]\n\t"
> "paddh  %[ftmp11],  %[ftmp4],   %[ftmp5]\n\t"
> -"xor%[ftmp7],   %[ftmp7],   %[ftmp7]\n\t"
> "psubh  %[ftmp5],   %[ftmp5],   %[ftmp4]\n\t"
> -MMI_SDC1(%[ftmp7], %[block], 0x00)
> -MMI_SDC1(%[ftmp7], %[block], 0x08)
> -MMI_SDC1(%[ftmp7], %[block], 0x10)
> -MMI_SDC1(%[ftmp7], %[block], 0x18)
> MMI_ULWC1(%[ftmp2], %[dst], 0x00)
> -"psrah  %[ftmp3],   

Re: [FFmpeg-devel] [PATCH 1/2] avcodec/lcldec: Optimize YUV422 case

[Reimar Döffinger]

On 28.07.2019, at 13:56, Michael Niedermayer  wrote:

> On Sun, Jul 28, 2019 at 12:45:36AM +0200, Reimar Döffinger wrote:
>> 
>> 
>> On 28.07.2019, at 00:31, Michael Niedermayer  wrote:
>> 
>>> This merges several byte operations and avoids some shifts inside the loop
>>> 
>>> Improves: Timeout (330sec -> 134sec)
>>> Improves: 
>>> 15599/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSZH_fuzzer-5658127116009472
>>> 
>>> Found-by: continuous fuzzing process 
>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>> Signed-off-by: Michael Niedermayer 
>>> ---
>>> libavcodec/lcldec.c | 10 +-
>>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>> 
>>> diff --git a/libavcodec/lcldec.c b/libavcodec/lcldec.c
>>> index 104defa5f5..c3787b3cbe 100644
>>> --- a/libavcodec/lcldec.c
>>> +++ b/libavcodec/lcldec.c
>>> @@ -391,13 +391,13 @@ static int decode_frame(AVCodecContext *avctx, void 
>>> *data, int *got_frame, AVPac
>>>break;
>>>case IMGTYPE_YUV422:
>>>for (row = 0; row < height; row++) {
>>> -for (col = 0; col < width - 3; col += 4) {
>>> +for (col = 0; col < (width - 2)>>1; col += 2) {
>>>memcpy(y_out + col, encoded, 4);
>>>encoded += 4;
>>> -u_out[ col >> 1 ] = *encoded++ + 128;
>>> -u_out[(col >> 1) + 1] = *encoded++ + 128;
>>> -v_out[ col >> 1 ] = *encoded++ + 128;
>>> -v_out[(col >> 1) + 1] = *encoded++ + 128;
>>> +AV_WN16(u_out + col, AV_RN16(encoded) ^ 0x8080);
>>> +encoded += 2;
>>> +AV_WN16(v_out + col, AV_RN16(encoded) ^ 0x8080);
>>> +encoded += 2;
>> 
>> Huh? Surely the pixel stride used for y_out still needs to be double of the 
>> u/v one?
> 
>> I suspect doing only the AV_RN16/xor optimization might be best, the one 
>> shift saved seems not worth the risk/complexity...
> 
> if you want i can remove the shift change ?
> with the fixed shift change its 155sec, if i remove the shift optimization 
> its 170sec
> 
> patch for the 155 case below:

I can't decide, it's a little uglier but a little faster...
Unless someone else has an opinion, go with whatever you prefer.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] avcodec/lcldec: Optimize YUV422 case

[Reimar Döffinger]

On 28.07.2019, at 00:31, Michael Niedermayer  wrote:

> This merges several byte operations and avoids some shifts inside the loop
> 
> Improves: Timeout (330sec -> 134sec)
> Improves: 
> 15599/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSZH_fuzzer-5658127116009472
> 
> Found-by: continuous fuzzing process 
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer 
> ---
> libavcodec/lcldec.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/libavcodec/lcldec.c b/libavcodec/lcldec.c
> index 104defa5f5..c3787b3cbe 100644
> --- a/libavcodec/lcldec.c
> +++ b/libavcodec/lcldec.c
> @@ -391,13 +391,13 @@ static int decode_frame(AVCodecContext *avctx, void 
> *data, int *got_frame, AVPac
> break;
> case IMGTYPE_YUV422:
> for (row = 0; row < height; row++) {
> -for (col = 0; col < width - 3; col += 4) {
> +for (col = 0; col < (width - 2)>>1; col += 2) {
> memcpy(y_out + col, encoded, 4);
> encoded += 4;
> -u_out[ col >> 1 ] = *encoded++ + 128;
> -u_out[(col >> 1) + 1] = *encoded++ + 128;
> -v_out[ col >> 1 ] = *encoded++ + 128;
> -v_out[(col >> 1) + 1] = *encoded++ + 128;
> +AV_WN16(u_out + col, AV_RN16(encoded) ^ 0x8080);
> +encoded += 2;
> +AV_WN16(v_out + col, AV_RN16(encoded) ^ 0x8080);
> +encoded += 2;

Huh? Surely the pixel stride used for y_out still needs to be double of the u/v 
one?
I suspect doing only the AV_RN16/xor optimization might be best, the one shift 
saved seems not worth the risk/complexity...
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v3] avutil/mips: Avoid instruction exception caused by gssqc1/gslqc1.

[Reimar Döffinger]

On 26.07.2019, at 07:18, Shiyou Yin  wrote:

> Ensure the address accesed by gssqc1/gslqc1 are 16-bits memory-aligned.

Looks good to me if standard DECLARE_ALIGNED should work for stack on MIPS.
(on x86 it used to be possible for the stack pointer to only be 8-byte aligned, 
in which case DECLARE_ALIGNED could not actually provide 16-byte aligned stack 
variables, so there were special macros - I have not checked how it works 
nowadays or if MIPS has that issue).
Well, I guess regardless of that, this is better than before, so should be fine 
to apply either way.

> ---
> libavcodec/mips/simple_idct_mmi.c | 2 +-
> libavutil/mips/mmiutils.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/mips/simple_idct_mmi.c 
> b/libavcodec/mips/simple_idct_mmi.c
> index 7f4bb74..73d797f 100644
> --- a/libavcodec/mips/simple_idct_mmi.c
> +++ b/libavcodec/mips/simple_idct_mmi.c
> @@ -39,7 +39,7 @@
> #define COL_SHIFT 20
> #define DC_SHIFT 3
> 
> -DECLARE_ALIGNED(8, const int16_t, W_arr)[46] = {
> +DECLARE_ALIGNED(16, const int16_t, W_arr)[46] = {
> W4,  W2,  W4,  W6,
> W1,  W3,  W5,  W7,
> W4,  W6, -W4, -W2,
> diff --git a/libavutil/mips/mmiutils.h b/libavutil/mips/mmiutils.h
> index 05f6b31..14b6d20 100644
> --- a/libavutil/mips/mmiutils.h
> +++ b/libavutil/mips/mmiutils.h
> @@ -205,7 +205,7 @@
>  * backup register
>  */
> #define BACKUP_REG \
> -  double temp_backup_reg[8];\
> +  DECLARE_ALIGNED(16, double, temp_backup_reg)[8];  \
>   if (_MIPS_SIM == _ABI64)  \
> __asm__ volatile (  \
>   "gssqc1   $f25,  $f24,   0x00(%[temp])  \n\t" \
> -- 
> 2.1.0
> 
> 
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v7 2/2] lavc/tiff: Decode embedded JPEGs in DNG images

[Reimar Döffinger]

On 26.07.2019, at 09:34, Nick Renieris  wrote:

> Στις Παρ, 26 Ιουλ 2019 στις 2:21 π.μ., ο/η Reimar Döffinger
>  έγραψε:
>> 
>> On 25.07.2019, at 17:35, velocit...@gmail.com wrote:
>> 
>>> +// Lookup table lookup
>>> +if (lut)
>>> +value = lut[value];
>> 
>> As this function is in the innermost loop, doing the if here instead of 
>> having 2 different implementations is likely not ideal speed-wise.
> 
> If this were C++ this'd be trivial, but as it stands I don't see a way
> to do this without sacrificing code readability and/or size.

Huh? Are you thinking of templates? always_inline can usually be used the same 
way.
I haven't looked into the how or anything, it was just a comment in principle.

> I believe branch prediction and instruction pipelining will hide this
> delay. I doubt it has any measurable effect on performance.

There are CPUs without that.

>>> +// Color scaling
>>> +value = av_clip_uint16_c((unsigned)(((float)value * scale_factor) * 
>>> 0x));
>> 
>> As input and output are both 16 bit I wonder if floating-point isn't rather 
>> overkill compared to doing fixed-point arithmetic.
> 
> Scaling in the [0.0, 1.0] range is mentioned in the DNG spec and it's
> also what dcraw does.

I don't see the connection? The point is as input and output are 16 bit this 
calculation can be done as integer operations without the need for floating 
point and all the conversion.
Depending on required precision with either 32 or 64 bit intermediate values.
Essentially by simply changing
(value * scale_factor) * 0x
to something along the lines of 
(value * (unsigned)(scale_factor * 0x * (1<<8))) >> 8
With of course most all but one multiply and shift outside the loop.
Of course would need to look at what the actual requirements are concerning 
precision, rounding and range. But that should be done anyway.

>>> +if (is_u16) {
>>> +for (line = 0; line < height; line++) {
>>> +uint16_t *dst_u16 = (uint16_t *)dst;
>>> +uint16_t *src_u16 = (uint16_t *)src;
>>> +
>>> +for (col = 0; col < width; col++)
>>> +*dst_u16++ = dng_raw_to_linear16(*src_u16++, s->dng_lut, 
>>> s->black_level, scale_factor);
>>> +
>>> +dst += dst_stride * sizeof(uint16_t);
>>> +src += src_stride * sizeof(uint16_t);
>> 
>> Is all this casting working correctly on e.g. big-endian?
> 
> Not sure, I don't see why not, considering I've seen such casting in
> other parts of ffmpeg.

Well, I did not find it obvious where src and dst are from and what format they 
are in.
Essentially if they are decoder output it's likely fine, if they are read from 
a file without decoding step it's likely wrong.

>> Also not sure if since these are essentially brightness/contrast adjustments 
>> if we should't rather just have a way to export the transform to use...
> 
> Export to where?

Just provide as metadata and leave to e.g. libavfilter.

> I don't see why you'd need to complicate this by
> calling into other components, the transformation code is concise,
> clear and accurate for this use case.

Slow and unoptimized and in it's current form hard to SIMD optimize, especially 
without changing output as well though (in addition to the large bit width of 
floats reducing the benefit, denormals can be an issue for SIMD-accelerating 
float code).
Unless I miss a reason why nobody would ever want this to be faster?

>>> @@ -1519,6 +1773,26 @@ again:
>>>return AVERROR_INVALIDDATA;
>>>}
>>> 
>>> +/* Handle DNG images with JPEG-compressed tiles */
>>> +
>>> +if (s->tiff_type == TIFF_TYPE_DNG || s->tiff_type == 
>>> TIFF_TYPE_CINEMADNG) {
>>> +if (s->is_jpeg) {
>>> +if (s->is_bayer) {
>>> +if ((ret = dng_decode(avctx, (AVFrame*)data, avpkt)) > 0)
>>> +*got_frame = 1;
>>> +return ret;
>>> +} else {
>>> +avpriv_report_missing_feature(avctx, "DNG JPG-compressed 
>>> non-bayer-encoded images");
>>> +return AVERROR_PATCHWELCOME;
>>> +}
>>> +} else if (s->is_tiled) {
>>> +avpriv_report_missing_feature(avctx, "DNG uncompressed tiled 
>>> images");
>>> +return AVERROR_PATCHWELCOME;
>>> +}
>> 
>> There is no need for an "else" block if the "if" block ends in a return.
> 
> Indeed, but in my opinion it makes the code clearer on first glance
> (if you miss the return). I can change it if you insist.

The second comment was the more relevant one actually.
I only really care about finding some way to make this part a whole lot more 
readable.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avcodec/mips: [loongson] refine process of setting block as 0 in h264dsp_mmi.

[Reimar Döffinger]

On 26.07.2019, at 10:25, Shiyou Yin  wrote:

> 1. Refine setting zero process in function ff_h264_add_pixels4_8_mmi and
>   ff_h264_idct_add_8_mmi.

"refine" is rather unspecific. What was changed? How large is the improvement, 
any specific numbers?

> 2. Remove redundant setting zeor

Typo

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] avcodec/vp3: Check that theora is theora

[Reimar Döffinger]

On 24.07.2019, at 14:37, Michael Niedermayer  wrote:

> On Mon, Jul 22, 2019 at 07:07:57AM +0200, Reimar Döffinger wrote:
>> 
>> 
>> On 22.07.2019, at 01:25, Michael Niedermayer  wrote:
>> 
>>> Fixes: Timeout (2min -> 100ms)
>>> Fixes: 
>>> 15366/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_THEORA_fuzzer-5737849938247680
>>> 
>>> Found-by: continuous fuzzing process 
>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>> Signed-off-by: Michael Niedermayer 
>>> ---
>>> libavcodec/vp3.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>> 
>>> diff --git a/libavcodec/vp3.c b/libavcodec/vp3.c
>>> index a6f759ebf5..8a165c1275 100644
>>> --- a/libavcodec/vp3.c
>>> +++ b/libavcodec/vp3.c
>>> @@ -2957,6 +2957,8 @@ static int theora_decode_header(AVCodecContext 
>>> *avctx, GetBitContext *gb)
>>>s->theora_header = 0;
>>>s->theora = get_bits_long(gb, 24);
>>>av_log(avctx, AV_LOG_DEBUG, "Theora bitstream version %X\n", s->theora);
>>> +if (!s->theora)
>>> +return AVERROR_INVALIDDATA;
>> 
>> That seems rather strict, a 1-bit error in this field and we don't even try?
>> Maybe set to 1 instead with a request for sample?
> 
> ok, will post a new patch

Thanks. Sorry if it was maybe a bit nitpicky comment, but seemed like doing it 
a but nicer might be worth the effort.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2] avutil/mips: Avoid instruction exception caused by gssqc1/gslqc1.

[Reimar Döffinger]

Is there a mips maintainer? otherwise:

On 24.07.2019, at 08:46, Shiyou Yin  wrote:

> Ensure the address accesed by gssqc1/gslqc1 are 16-bits memory-aligned.
> ---
> libavcodec/mips/simple_idct_mmi.c | 2 +-
> libavutil/mips/mmiutils.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/mips/simple_idct_mmi.c 
> b/libavcodec/mips/simple_idct_mmi.c
> index 7f4bb74..73d797f 100644
> --- a/libavcodec/mips/simple_idct_mmi.c
> +++ b/libavcodec/mips/simple_idct_mmi.c
> @@ -39,7 +39,7 @@
> #define COL_SHIFT 20
> #define DC_SHIFT 3
> 
> -DECLARE_ALIGNED(8, const int16_t, W_arr)[46] = {
> +DECLARE_ALIGNED(16, const int16_t, W_arr)[46] = {
> W4,  W2,  W4,  W6,
> W1,  W3,  W5,  W7,
> W4,  W6, -W4, -W2,

This should be fine, simply as it should not be possible for it to cause issues.

> diff --git a/libavutil/mips/mmiutils.h b/libavutil/mips/mmiutils.h
> index 05f6b31..bfa6d8b 100644
> --- a/libavutil/mips/mmiutils.h
> +++ b/libavutil/mips/mmiutils.h
> @@ -205,7 +205,7 @@
>  * backup register
>  */
> #define BACKUP_REG \
> -  double temp_backup_reg[8];\
> +  double __attribute__ ((aligned (16))) temp_backup_reg[8]; \


I don't think we're supposed to use raw __attribute__ in FFmpeg, and for stack 
variables there can be even more issues.
Maybe check with what other code does...
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v7 2/2] lavc/tiff: Decode embedded JPEGs in DNG images

[Reimar Döffinger]

On 25.07.2019, at 17:35, velocit...@gmail.com wrote:

> +// Lookup table lookup
> +if (lut)
> +value = lut[value];

As this function is in the innermost loop, doing the if here instead of having 
2 different implementations is likely not ideal speed-wise.

> +// Color scaling
> +value = av_clip_uint16_c((unsigned)(((float)value * scale_factor) * 
> 0x));

As input and output are both 16 bit I wonder if floating-point isn't rather 
overkill compared to doing fixed-point arithmetic.

> 
> +if (is_u16) {
> +for (line = 0; line < height; line++) {
> +uint16_t *dst_u16 = (uint16_t *)dst;
> +uint16_t *src_u16 = (uint16_t *)src;
> +
> +for (col = 0; col < width; col++)
> +*dst_u16++ = dng_raw_to_linear16(*src_u16++, s->dng_lut, 
> s->black_level, scale_factor);
> +
> +dst += dst_stride * sizeof(uint16_t);
> +src += src_stride * sizeof(uint16_t);

Is all this casting working correctly on e.g. big-endian?
Also using sizeof on uint16_t and uint8_t seems a bit overkill.
Also not sure if since these are essentially brightness/contrast adjustments if 
we should't rather just have a way to export the transform to use...


> @@ -1519,6 +1773,26 @@ again:
> return AVERROR_INVALIDDATA;
> }
> 
> +/* Handle DNG images with JPEG-compressed tiles */
> +
> +if (s->tiff_type == TIFF_TYPE_DNG || s->tiff_type == 
> TIFF_TYPE_CINEMADNG) {
> +if (s->is_jpeg) {
> +if (s->is_bayer) {
> +if ((ret = dng_decode(avctx, (AVFrame*)data, avpkt)) > 0)
> +*got_frame = 1;
> +return ret;
> +} else {
> +avpriv_report_missing_feature(avctx, "DNG JPG-compressed 
> non-bayer-encoded images");
> +return AVERROR_PATCHWELCOME;
> +}
> +} else if (s->is_tiled) {
> +avpriv_report_missing_feature(avctx, "DNG uncompressed tiled 
> images");
> +return AVERROR_PATCHWELCOME;
> +}

There is no need for an "else" block if the "if" block ends in a return.
Also putting the error handling first/at the deepest indentation level results 
in more readable code generally.

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avcodec/rl2: set dimensions

[Reimar Döffinger]

On 24.07.2019, at 02:39, Kieran Kunhya  wrote:

>> 
>> What was the cause of the slow decoding? Does this actually fix it, or
>> does it just swipe it "under the carpet"?
>> If someone ever found a sample with a different solution, how would they
>> know that they shouldn't just remove this again? Without any kind of
>> comment on the point of this call, people might assume it's pointless
>> nonsense.
>> 
> 
> A significant proportion of these patches sweep the issue under the carpet.

Which is not necessarily the wrong choice.
But by leaving no documentation a lot of the time spend on writing the patches 
is wasted since the knowledge gained is just lost, maintainers need to 
"reverse-engineer" them etc.

> Not to mention the swathes of annoyed developers

And let's not turn this into a conflict, I just tried to give clear feedback 
why I am unhappy
with some of these patches even though they may be worthwhile still.
> 

Hopefully some of that also captures reasons others feel unhappy (and it would 
be nice
if the concerns could be raised more constructively, though I do acknowledge it 
can be hard).
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avcodec/rl2: set dimensions

[Reimar Döffinger]

On 22.07.2019, at 23:57, Michael Niedermayer  wrote:

> The dimensions are always 320x200 they are hardcoded in the demuxer.
> Hardcode them instead in the decoder.
> 
> Fixes: Timeout (16sec -> 400ms)
> Fixes: 
> 15574/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RL2_fuzzer-5158614072819712
> 
> Found-by: continuous fuzzing process 
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer 
> ---
> libavcodec/rl2.c | 5 +
> 1 file changed, 5 insertions(+)
> 
> diff --git a/libavcodec/rl2.c b/libavcodec/rl2.c
> index 6662979c52..2d336a61e5 100644
> --- a/libavcodec/rl2.c
> +++ b/libavcodec/rl2.c
> @@ -134,10 +134,15 @@ static av_cold int rl2_decode_init(AVCodecContext 
> *avctx)
> Rl2Context *s = avctx->priv_data;
> int back_size;
> int i;
> +int ret;
> 
> s->avctx   = avctx;
> avctx->pix_fmt = AV_PIX_FMT_PAL8;
> 
> +ret = ff_set_dimensions(avctx, 320, 200);
> +if (ret < 0)
> +return ret;

I really dislike these completely comment-less patches.
So it seems this is only based on what our demuxer does.
However does the format itself have any inherent size limitations?
What was the cause of the slow decoding? Does this actually fix it, or does it 
just swipe it "under the carpet"?
If someone ever found a sample with a different solution, how would they know 
that they shouldn't just remove this again? Without any kind of comment on the 
point of this call, people might assume it's pointless nonsense.

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avutil/mips: Avoid instruction exception caused by gssqc1/gslqc1.

[Reimar Döffinger]

Why is "block" not aligned? Does the code for other architectures also use 
unaligned instructions for these?

On 23.07.2019, at 09:27, Shiyou Yin  wrote:

> Ensure the address accesed by gssqc1/gslqc1 are 16-bits memory-aligned.
> ---
> libavcodec/mips/h264dsp_mmi.c | 48 +---
> libavcodec/mips/simple_idct_mmi.c | 51 +--
> libavutil/mips/mmiutils.h |  2 +-
> 3 files changed, 51 insertions(+), 50 deletions(-)
> 
> diff --git a/libavcodec/mips/h264dsp_mmi.c b/libavcodec/mips/h264dsp_mmi.c
> index ac65a20..a85d782 100644
> --- a/libavcodec/mips/h264dsp_mmi.c
> +++ b/libavcodec/mips/h264dsp_mmi.c
> @@ -38,6 +38,11 @@ void ff_h264_add_pixels4_8_mmi(uint8_t *dst, int16_t *src, 
> int stride)
> MMI_LDC1(%[ftmp2], %[src], 0x08)
> MMI_LDC1(%[ftmp3], %[src], 0x10)
> MMI_LDC1(%[ftmp4], %[src], 0x18)
> +/* memset(src, 0, 32); */
> +MMI_USDC1(%[ftmp0], %[src], 0x00)
> +MMI_USDC1(%[ftmp0], %[src], 0x08)
> +MMI_USDC1(%[ftmp0], %[src], 0x10)
> +MMI_USDC1(%[ftmp0], %[src], 0x18)
> MMI_ULWC1(%[ftmp5], %[dst0], 0x00)
> MMI_ULWC1(%[ftmp6], %[dst1], 0x00)
> MMI_ULWC1(%[ftmp7], %[dst2], 0x00)
> @@ -58,11 +63,6 @@ void ff_h264_add_pixels4_8_mmi(uint8_t *dst, int16_t *src, 
> int stride)
> MMI_SWC1(%[ftmp2], %[dst1], 0x00)
> MMI_SWC1(%[ftmp3], %[dst2], 0x00)
> MMI_SWC1(%[ftmp4], %[dst3], 0x00)
> -
> -/* memset(src, 0, 32); */
> -"xor%[ftmp0],   %[ftmp0],   %[ftmp0]\n\t"
> -"gssqc1 %[ftmp0],   %[ftmp0],   0x00(%[src])\n\t"
> -"gssqc1 %[ftmp0],   %[ftmp0],   0x10(%[src])\n\t"
> : [ftmp0]"="(ftmp[0]),[ftmp1]"="(ftmp[1]),
>   [ftmp2]"="(ftmp[2]),[ftmp3]"="(ftmp[3]),
>   [ftmp4]"="(ftmp[4]),[ftmp5]"="(ftmp[5]),
> @@ -85,15 +85,21 @@ void ff_h264_idct_add_8_mmi(uint8_t *dst, int16_t *block, 
> int stride)
> DECLARE_VAR_ADDRT;
> 
> __asm__ volatile (
> -"dli%[tmp0],0x01\n\t"
> MMI_LDC1(%[ftmp0], %[block], 0x00)
> -"mtc1   %[tmp0],%[ftmp8]\n\t"
> MMI_LDC1(%[ftmp1], %[block], 0x08)
> -"dli%[tmp0],0x06\n\t"
> MMI_LDC1(%[ftmp2], %[block], 0x10)
> +MMI_LDC1(%[ftmp3], %[block], 0x18)
> +/* memset(block, 0, 32) */
> +"xor%[ftmp4],   %[ftmp4],   %[ftmp4]\n\t"
> +MMI_USDC1(%[ftmp4], %[block], 0x00)
> +MMI_USDC1(%[ftmp4], %[block], 0x08)
> +MMI_USDC1(%[ftmp4], %[block], 0x10)
> +MMI_USDC1(%[ftmp4], %[block], 0x18)
> +"dli%[tmp0],0x01\n\t"
> +"mtc1   %[tmp0],%[ftmp8]\n\t"
> +"dli%[tmp0],0x06\n\t"
> "mtc1   %[tmp0],%[ftmp9]\n\t"
> "psrah  %[ftmp4],   %[ftmp1],   %[ftmp8]\n\t"
> -MMI_LDC1(%[ftmp3], %[block], 0x18)
> "psrah  %[ftmp5],   %[ftmp3],   %[ftmp8]\n\t"
> "psubh  %[ftmp4],   %[ftmp4],   %[ftmp3]\n\t"
> "paddh  %[ftmp5],   %[ftmp5],   %[ftmp1]\n\t"
> @@ -121,15 +127,11 @@ void ff_h264_idct_add_8_mmi(uint8_t *dst, int16_t 
> *block, int stride)
> "paddh  %[ftmp10],  %[ftmp3],   %[ftmp1]\n\t"
> "psubh  %[ftmp1],   %[ftmp1],   %[ftmp3]\n\t"
> "paddh  %[ftmp11],  %[ftmp4],   %[ftmp5]\n\t"
> -"xor%[ftmp7],   %[ftmp7],   %[ftmp7]\n\t"
> "psubh  %[ftmp5],   %[ftmp5],   %[ftmp4]\n\t"
> -MMI_SDC1(%[ftmp7], %[block], 0x00)
> -MMI_SDC1(%[ftmp7], %[block], 0x08)
> -MMI_SDC1(%[ftmp7], %[block], 0x10)
> -MMI_SDC1(%[ftmp7], %[block], 0x18)
> MMI_ULWC1(%[ftmp2], %[dst], 0x00)
> -"psrah  %[ftmp3],   %[ftmp10],  %[ftmp9]\n\t"
> MMI_LWXC1(%[ftmp0], %[dst], %[stride], 0x00)
> +"xor%[ftmp7],   %[ftmp7],   %[ftmp7]\n\t"
> +"psrah  %[ftmp3],   %[ftmp10],  %[ftmp9]\n\t"
> "psrah  %[ftmp4],   %[ftmp11],  %[ftmp9]\n\t"
> "punpcklbh  %[ftmp2],   %[ftmp2],   %[ftmp7]\n\t"
> "punpcklbh  %[ftmp0],   %[ftmp0],   %[ftmp7]\n\t"
> @@ -153,11 +155,6 @@ void ff_h264_idct_add_8_mmi(uint8_t *dst, int16_t 
> *block, int stride)
> MMI_SWC1(%[ftmp2], %[dst], 0x00)
> "packushb   %[ftmp0],   %[ftmp0],   %[ftmp7]

Re: [FFmpeg-devel] [PATCH 2/2] avcodec/vp3: Check that theora is theora

[Reimar Döffinger]

On 22.07.2019, at 01:25, Michael Niedermayer  wrote:

> Fixes: Timeout (2min -> 100ms)
> Fixes: 
> 15366/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_THEORA_fuzzer-5737849938247680
> 
> Found-by: continuous fuzzing process 
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer 
> ---
> libavcodec/vp3.c | 2 ++
> 1 file changed, 2 insertions(+)
> 
> diff --git a/libavcodec/vp3.c b/libavcodec/vp3.c
> index a6f759ebf5..8a165c1275 100644
> --- a/libavcodec/vp3.c
> +++ b/libavcodec/vp3.c
> @@ -2957,6 +2957,8 @@ static int theora_decode_header(AVCodecContext *avctx, 
> GetBitContext *gb)
> s->theora_header = 0;
> s->theora = get_bits_long(gb, 24);
> av_log(avctx, AV_LOG_DEBUG, "Theora bitstream version %X\n", s->theora);
> +if (!s->theora)
> +return AVERROR_INVALIDDATA;

That seems rather strict, a 1-bit error in this field and we don't even try?
Maybe set to 1 instead with a request for sample?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v4 2/3] lavc/libdavs2.c: fix decoder info level setting

[Reimar Döffinger]

On 22.07.2019, at 06:23, hwrenx  wrote:

> Mapping log level from av_log_level to davs3_log_level_e:
> 
> [AV_LOG_QUIET, AV_LOG_ERROR]   => DAVS2_LOG_ERROR
> [AV_LOG_WARNING]   => DAVS2_LOG_WARNING
> [AV_LOG_INFO]  => DAVS2_LOG_INFO
> [AV_LOG_VERBOSE, AV_LOG_TRACE] => DAVS2_LOG_DEBUG
> 
> in values:
> 
> [-8, 16] => 3
> [17, 24] => 2
> [25, 32] => 1
> [33, 56] => 0
> 
> After clip into [AV_LOG_FATAL + 1, AV_LOG_VERBOSE]([9, 40]), davs2 log
> level can be expressed as (4-(((av_log_level)-1)>>3)).

The AV_LOG_... values might change at some point.
I'd suggest using a couple of ifs instead of trying something "clever" like 
this.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [FFmpeg-cvslog] avformat/aacdec: factorize the adts frame resync code

[Reimar Döffinger]

On 21.07.2019, at 02:51, James Almer  wrote:

> ffmpeg | branch: master | James Almer  | Sat Jul 20 
> 10:13:08 2019 -0300| [a38eab8b7501440f872ff1af8a0c5482b7b3e532] | committer: 
> James Almer
> 
> avformat/aacdec: factorize the adts frame resync code
> 
> Signed-off-by: James Almer 
> 
>> http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=a38eab8b7501440f872ff1af8a0c5482b7b3e532
> ---
> 
> libavformat/aacdec.c | 37 +
> 1 file changed, 25 insertions(+), 12 deletions(-)
> 
> diff --git a/libavformat/aacdec.c b/libavformat/aacdec.c
> index 8a5450880b..262614fdd9 100644
> --- a/libavformat/aacdec.c
> +++ b/libavformat/aacdec.c
> @@ -80,10 +80,31 @@ static int adts_aac_probe(const AVProbeData *p)
> return 0;
> }
> 
> +static int adts_aac_resync(AVFormatContext *s)
> +{
> +uint16_t state;
> +
> +// skip data until an ADTS frame is found
> +state = avio_r8(s->pb);

Nit: could have merged declaration an initialization here.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 6/6] avcodec/flicvideo: More strictly check chunk size for FLI_COPY

[Reimar Döffinger]

On 19.07.2019, at 21:46, Michael Niedermayer  wrote:

> On Fri, Jul 19, 2019 at 03:54:19PM +0200, Paul B Mahol wrote:
>> On 7/19/19, Michael Niedermayer  wrote:
>>> On Sat, Jun 22, 2019 at 01:29:36AM +0200, Michael Niedermayer wrote:
 Fixes: Timeout (40sec -> 13sec)
 Fixes:
 15417/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FLIC_fuzzer-5679812615602176
 
 Found-by: continuous fuzzing process
 https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
 Signed-off-by: Michael Niedermayer 
 ---
 libavcodec/flicvideo.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
>>> 
>>> will apply
>>> 
>> 
>> Why? This actually is breaking old code.
>> Can you please stop committing such kind of patches?
> 
> This patch was on the mailing list since a month, why do you point
> out a problem with it only once i say that i intend to apply it ?

You are sending an awful lot of these patches.
For my part it is purely random whether and when I manage to look at one even 
for code I somewhat know.
I imagine a lot of other people are struggling to keep up as well.
If you want better reviews you might want to think about better ways to 
categorise them to reduce review burden - e.g. by risk level.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/4] avcodec/hqx: Check the input data against the image size

[Reimar Döffinger]

On 21.07.2019, at 00:36, Lynne  wrote:

> Jul 20, 2019, 11:08 PM by mich...@niedermayer.cc:
> 
>> Fixes: Timeout (22 -> 7 sec)
>> Fixes: 
>> 15173/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HQX_fuzzer-5662556846292992
>> 
>> Found-by: continuous fuzzing process 
>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> Signed-off-by: Michael Niedermayer 
>> ---
>> libavcodec/hqx.c | 4 
>> 1 file changed, 4 insertions(+)
>> 
>> diff --git a/libavcodec/hqx.c b/libavcodec/hqx.c
>> index bc24ba91d1..8639d77a41 100644
>> --- a/libavcodec/hqx.c
>> +++ b/libavcodec/hqx.c
>> @@ -471,6 +471,10 @@ static int hqx_decode_frame(AVCodecContext *avctx, void 
>> *data,
>> avctx->height  = ctx->height;
>> avctx->bits_per_raw_sample = 10;
>> 
>> +if (avctx->coded_width / 16 * (avctx->coded_height / 16) *
>> +(100 - avctx->discard_damaged_percentage) / 100 > 8LL * avpkt->size)
>> +return AVERROR_INVALIDDATA;
>> + 
>> 
> 
> Not only are you ignoring my and others opinion, not only you still continue 
> sending these awful patches,
> you've just submitted by far the worst one I've ever seen thinking its okay.
> Patches like these motivate developers to not even bother including test 
> samples for new decoders, or even write them. Myself included. Doing exactly 
> the opposite of what this system's meant to help.
> Sure, you sent this for review, but how can you even consider this utterly 
> ridiculous hack for a problem that doesn't exist even worthy for review in 
> the first place? Just what the fuck?

I kind of understand your point of view, and the fuzzer complaining should not 
be an excuse to skip writing a commit message with some motivation for example, 
but I think you are a bit over the top.
There is already a discard_damaged_percentage and there is a point that maybe 
if a packet is obviously too broken to discard it with minimal overhead.
Those do not seem like utterly ridiculous ideas as your reply makes them out to 
me.
Nor is the idea of having some hardening against all too easy DoS attacks in 
some use-cases.
Also some value in just having the fuzzer run efficiently (it also discovers 
quite some real issues).
I agree it's hackish, I don't know the code enough to know it's correct, it 
needs to be properly documented (Michael, please, if you add such non-obvious, 
and especially heuristic checks, there needs to be some comment telling people 
the idea behind it and how to easily verify its correctness etc.).
With that in mind, can you maybe see why I do think that discussing such patch 
proposals does have merit?
Can we maybe come up with some compromise without being mad at each other?
Maybe some of these likely to be more controversial could be submitted as RFC 
instead of patch first?

Best regards,
Reimar
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] avcodec/fitsdec: Prevent division by 0 with huge data_max

[Reimar Döffinger]

On 18.07.2019, at 12:54, Michael Niedermayer  wrote:

> On Thu, Jul 18, 2019 at 08:21:21AM +0200, Reimar Döffinger wrote:
>> 
>> 
>> On 16.07.2019, at 20:31, Michael Niedermayer  wrote:
>> 
>>> On Tue, Jul 16, 2019 at 08:34:14AM +0200, Reimar Döffinger wrote:
>>>> On 16.07.2019, at 00:50, Michael Niedermayer  
>>>> wrote:
>>>> 
>>>>> Fixes: division by 0
>>>>> Fixes: 
>>>>> 15657/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FITS_fuzzer-5738154838982656
>>>>> 
>>>>> Found-by: continuous fuzzing process 
>>>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>>> Signed-off-by: Michael Niedermayer 
>>>>> ---
>>>>> libavcodec/fitsdec.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>> 
>>>>> diff --git a/libavcodec/fitsdec.c b/libavcodec/fitsdec.c
>>>>> index 4f452422ef..fe941f199d 100644
>>>>> --- a/libavcodec/fitsdec.c
>>>>> +++ b/libavcodec/fitsdec.c
>>>>> @@ -174,7 +174,7 @@ static int fits_read_header(AVCodecContext *avctx, 
>>>>> const uint8_t **ptr, FITSHead
>>>>>   return AVERROR_INVALIDDATA;
>>>>>   }
>>>>>   av_log(avctx, AV_LOG_WARNING, "data min/max indicates a blank 
>>>>> image\n");
>>>>> -header->data_max ++;
>>>>> +header->data_max += fabs(header->data_max) / 1000 + 1;
>>>> 
>>>> This is really non-obvious, both by itself, in where/how it causes the 
>>>> division by 0 and that the error here isn't worse than the division by 0 
>>>> for example, and why this constant was chosen.
>>> 
>>> division by 0 occured in:
>>> *dst++ = ((t - header.data_min) * ((1 << (sizeof(type) * 8)) - 1)) / 
>>> (header.data_max - header.data_min); \
>> 
>> I looked at the code, and now it makes even less sense to me.
>> First, why is that reported as an error at all?
>> Dividing by 0 is well defined for floating-point.
> 
> at least at the point where its stored in an integer it becomes painfull
> to the compiler to find a way to do it.

Hm, I am not quite following. The division results in inf or nan, and those get 
converted to integer the usual way (not sure how well that is defined in C, but 
it's not a division by 0 error).

> Iam not sure if inf is a problem (from a very quick look) that would get
> divided to 0 i guess nan would be an issue, i didnt think of this, i will 
> redo this and post a better patch

inf / inf = nan
From my point of view if 0.0 / 0.0 is considered an issue that seems like it 
should apply for inf as well.
When it comes to actually correct code, there might also be the question what 
to do in that case.
Could be considered "bad data/normalization range, just skip the whole scaling".
Or could define it like OpenGL (and other) normalization operations:
-inf becomes -1, inf becomes 1 all else 0. But that would need a special code 
case, and I guess it's not worth it.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] avcodec/fitsdec: Prevent division by 0 with huge data_max

[Reimar Döffinger]

On 16.07.2019, at 20:31, Michael Niedermayer  wrote:

> On Tue, Jul 16, 2019 at 08:34:14AM +0200, Reimar Döffinger wrote:
>> On 16.07.2019, at 00:50, Michael Niedermayer  wrote:
>> 
>>> Fixes: division by 0
>>> Fixes: 
>>> 15657/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FITS_fuzzer-5738154838982656
>>> 
>>> Found-by: continuous fuzzing process 
>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>> Signed-off-by: Michael Niedermayer 
>>> ---
>>> libavcodec/fitsdec.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/libavcodec/fitsdec.c b/libavcodec/fitsdec.c
>>> index 4f452422ef..fe941f199d 100644
>>> --- a/libavcodec/fitsdec.c
>>> +++ b/libavcodec/fitsdec.c
>>> @@ -174,7 +174,7 @@ static int fits_read_header(AVCodecContext *avctx, 
>>> const uint8_t **ptr, FITSHead
>>>return AVERROR_INVALIDDATA;
>>>}
>>>av_log(avctx, AV_LOG_WARNING, "data min/max indicates a blank 
>>> image\n");
>>> -header->data_max ++;
>>> +header->data_max += fabs(header->data_max) / 1000 + 1;
>> 
>> This is really non-obvious, both by itself, in where/how it causes the 
>> division by 0 and that the error here isn't worse than the division by 0 for 
>> example, and why this constant was chosen.
> 
> division by 0 occured in:
> *dst++ = ((t - header.data_min) * ((1 << (sizeof(type) * 8)) - 1)) / 
> (header.data_max - header.data_min); \

I looked at the code, and now it makes even less sense to me.
First, why is that reported as an error at all?
Dividing by 0 is well defined for floating-point.
Next, your patch handles only one corner-case while not handling others.
For example, data_min and data_max can also be inf or NaN, which equally make 
no sense (and result in a division by NaN, which can hardly be better than a 
division by 0).
Next, bzero is applied to data_min and data_max which can cause precision 
issues, so it's a bit questionable but maybe non-trivial to fix completely.
However as data_max is never used but only the delta, most of these issues can 
be fixed much more thoroughly (and improve performance) by calculating and 
storing only that delta, and before applying bzero. Then delta can simply be 
overridden to 1 if it is fishy (not a normal or 0).
Of course there is a question if values above data_max are supposed to result 
in output > 1 or be clamped, but I guess that issue can be ignored.
As the code pays no particular attention to precision issue it would also be a 
question if calculating the inverse and use multiplications instead of 
divisions would make sense, but that admittedly is just an optimization.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] avcodec/fitsdec: Prevent division by 0 with huge data_max

[Reimar Döffinger]

On 16.07.2019, at 00:50, Michael Niedermayer  wrote:

> Fixes: division by 0
> Fixes: 
> 15657/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FITS_fuzzer-5738154838982656
> 
> Found-by: continuous fuzzing process 
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer 
> ---
> libavcodec/fitsdec.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavcodec/fitsdec.c b/libavcodec/fitsdec.c
> index 4f452422ef..fe941f199d 100644
> --- a/libavcodec/fitsdec.c
> +++ b/libavcodec/fitsdec.c
> @@ -174,7 +174,7 @@ static int fits_read_header(AVCodecContext *avctx, const 
> uint8_t **ptr, FITSHead
> return AVERROR_INVALIDDATA;
> }
> av_log(avctx, AV_LOG_WARNING, "data min/max indicates a blank 
> image\n");
> -header->data_max ++;
> +header->data_max += fabs(header->data_max) / 1000 + 1;

This is really non-obvious, both by itself, in where/how it causes the division 
by 0 and that the error here isn't worse than the division by 0 for example, 
and why this constant was chosen.
Also why a division and not a multiply by the inverse?
Why not * (1.0f / (1 << 24)) for example, which for single-precision IEEE I 
think should result in exactly 1 ULP (well, possibly 2 with rounding) 
increments?
Why is this even using floating-point? And why not double-precision at least?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] avcodec/sanm: Check extradata_size before allocations

[Reimar Döffinger]

On 16.07.2019, at 00:50, Michael Niedermayer  wrote:

> Fixes: Leaks
> Fixes: 
> 15349/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_SANM_fuzzer-5102530557640704
> 
> Found-by: continuous fuzzing process 
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer 
> ---
> libavcodec/sanm.c | 9 -
> 1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/libavcodec/sanm.c b/libavcodec/sanm.c
> index 25aee7220f..60e2f4c624 100644
> --- a/libavcodec/sanm.c
> +++ b/libavcodec/sanm.c
> @@ -491,6 +491,10 @@ static av_cold int decode_init(AVCodecContext *avctx)
> 
> ctx->avctx   = avctx;
> ctx->version = !avctx->extradata_size;
> +if (!ctx->version && avctx->extradata_size < 1026) {
> +av_log(avctx, AV_LOG_ERROR, "Not enough extradata.\n");
> +return AVERROR_INVALIDDATA;
> +}
> 
> avctx->pix_fmt = ctx->version ? AV_PIX_FMT_RGB565 : AV_PIX_FMT_PAL8;
> 
> @@ -506,11 +510,6 @@ static av_cold int decode_init(AVCodecContext *avctx)
> if (!ctx->version) {
> int i;
> 
> -if (avctx->extradata_size < 1026) {
> -av_log(avctx, AV_LOG_ERROR, "Not enough extradata.\n");
> -return AVERROR_INVALIDDATA;
> -}

This seems quite a bit less obvious.
Is that the only error return case, and is adding the cleanup code complex 
enough that this is the better choice?
Either way I'd recommend a comment like
// early sanity check before allocations to avoid need for deallocation code.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v6] Fix integer parameters size check in SDP fmtp line

[Reimar Döffinger]

This seems reasonable to me, but I have not been involved in any rtp code.
The commit message is maybe a bit on the verbose side, but I don't have any 
strong opinion.

On 12.07.2019, at 08:40, Olivier Maignial  wrote:

> === PROBLEM ===
> 
> I was trying to record h264 + aac streams from an RTSP server to mp4 file. 
> using this command line:
>ffmpeg -v verbose -y -i "rtsp:///my_resources" -codec copy -bsf:a 
> aac_adtstoasc test.mp4
> 
> FFmpeg then fail to record audio and output this logs:
>[rtsp @ 0xcda1f0] The profile-level-id field size is invalid (40)
>[rtsp @ 0xcda1f0] Error parsing AU headers
>...
>[rtsp @ 0xcda1f0] Could not find codec parameters for stream 1 (Audio: 
> aac, 48000 Hz, 1 channels): unspecified sample format
> 
> In SDP provided by my RTSP server I had this fmtp line:
>a=fmtp:98 streamType=5; profile-level-id=40; mode=AAC-hbr; config=1188; 
> sizeLength=13; indexLength=3; indexDeltaLength=3;
> 
> In FFmpeg code, I found a check introduced by commit 
> 24130234cd9dd733116d17b724ea4c8e12ce097a. It disallows values greater than 32 
> for fmtp line parameters.
> However, In RFC-6416 (RTP Payload Format for MPEG-4 Audio/Visual Streams) 
> give examples of "profile-level-id" values for AAC, up to 55.
> Furthermore, RFC-4566 (SDP: Session Description Protocol) do not give any 
> limit of size on interger parameters given in fmtp line.
> 
> === FIX ===
> 
> Instead of prohibit values over 32, I propose to check the possible integer 
> overflow.
> The use of strtoll allow to check the string validity and the possible 
> overflow.
> Value is then checked against INT32_MIN and INT32_MAX. Using INT32_MIN/MAX 
> ensure to have the same behavior on 32 or 64 bits platforms.
> 
> This patch fix my problem and I now can record my RTSP AAC stream to mp4.
> It has passed the full fate tests suite sucessfully.
> 
> Signed-off-by: Olivier Maignial 
> ---
> 
> Changes V6 --> V7:
>- Use long long int and strtoll. LLONG_MAX is always greather than 
> INT32_MIN while LONG_MAX can be equal to INT32_MAX. It allows to accept full 
> INT32 range.
>- Avoid to use errno
> 
> libavformat/rtpdec_mpeg4.c | 13 -
> 1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/libavformat/rtpdec_mpeg4.c b/libavformat/rtpdec_mpeg4.c
> index 4f70599..f1cbedf 100644
> --- a/libavformat/rtpdec_mpeg4.c
> +++ b/libavformat/rtpdec_mpeg4.c
> @@ -289,15 +289,18 @@ static int parse_fmtp(AVFormatContext *s,
> for (i = 0; attr_names[i].str; ++i) {
> if (!av_strcasecmp(attr, attr_names[i].str)) {
> if (attr_names[i].type == ATTR_NAME_TYPE_INT) {
> -int val = atoi(value);
> -if (val > 32) {
> +char *end_ptr = NULL;
> +long long int val = strtoll(value, _ptr, 10);
> +if (end_ptr == value || end_ptr[0] != '\0' ||
> +val < INT32_MIN || val > INT32_MAX) {
> av_log(s, AV_LOG_ERROR,
> -   "The %s field size is invalid (%d)\n",
> -   attr, val);
> +   "The %s field value is not a valid number, or 
> overflows int32: %s\n",
> +   attr, value);
> return AVERROR_INVALIDDATA;
> }
> +
> *(int *)((char *)data+
> -attr_names[i].offset) = val;
> +attr_names[i].offset) = (int) val;
> } else if (attr_names[i].type == ATTR_NAME_TYPE_STR) {
> char *val = av_strdup(value);
> if (!val)
> -- 
> 2.7.4
> 
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/6] avformat/vividas: Fixes overflow in shift in recover_key()

[Reimar Döffinger]

On 13 July 2019 22:25:02 CEST, Michael Niedermayer  
wrote:
>Fixes: left shift of 133 by 24 places cannot be represented in type
>'int'
>Fixes:
>15365/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5716153105645568
>
>Found-by: continuous fuzzing process
>https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>Suggested-by: Reimar Döffinger 
>Signed-off-by: Michael Niedermayer 
>---
> libavformat/vividas.c | 5 +
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
>diff --git a/libavformat/vividas.c b/libavformat/vividas.c
>index 350c7aa70a..830e318645 100644
>--- a/libavformat/vividas.c
>+++ b/libavformat/vividas.c
>@@ -115,10 +115,7 @@ static unsigned recover_key(unsigned char
>sample[4], unsigned expected_size)
> 
> put_v(plaintext+2, expected_size);
> 
>-return (sample[0]^plaintext[0])|
>-((sample[1]^plaintext[1])<<8)|
>-((sample[2]^plaintext[2])<<16)|
>-((sample[3]^plaintext[3])<<24);
>+return AV_RL32(sample) ^ AV_RL32(plaintext);
> }
> 
>static void xor_block(void *p1, void *p2, unsigned size, int key,
>unsigned *key_ptr)

Looks good to me.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v6] Fix integer parameters size check in SDP fmtp line

[Reimar Döffinger]

On 03.07.2019, at 09:28, Olivier MAIGNIAL  wrote:

> Hello, thanks for review,
> 
> 1) Can you give some reason about why we shouldn't use errno? I think it is
> more clear to accept the full int32 range, even if min/max int32 values are
> very unlikely to be used.

It is a global variable, with all the issues that has, also for code clarity 
(to review code you need to know which functions modify it).
On some systems it is not even thread-local, so using it becomes completely 
unsafe once threads are involved, but even when it is, signal handlers can by 
accident change it.
I see the argument that these are rare, special and broken cases, but it's not 
like using errno gains all that much (esp. when using stroll is also an 
alternative that avoids the need).
The shorter version of that argument is that errno is a major misdesign beyond 
even strcpy and strncpy levels, which I personally find good reason to avoid it 
where reasonable.
Note that there is also the issue that "long" is 32 bit on 32 bit systems, I 
would expect the if condition to trigger compiler warnings there, which strtoll 
should avoid.

> 2) The RFC 4566 don't give any limit on fmtp parameters values. In addition
> I can't find a spec that says fmtp integers parameters for AAC must be
> positive. So I don't think we should limit values to positive integers here
> as it would not be consistent to RFC.

I admit I have not found any requirement on it being integer at all, so not 
sure if that assumption is not wrong to start with and should at least not 
trigger an error.
And nothing that would make > 32 bit wrong.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avutil: add av_memcpy() to avoid undefined behavior with NULL, NULL, 0

[Reimar Döffinger]

On 03.07.2019, at 08:29, Michael Niedermayer  wrote:

> On Tue, Jul 02, 2019 at 08:42:43PM -0300, James Almer wrote:
>> On 7/2/2019 5:56 PM, Michael Niedermayer wrote:
>>> Signed-off-by: Michael Niedermayer 
>>> ---
>>> doc/APIchanges  |  3 +++
>>> libavutil/mem.h | 13 +
>>> libavutil/version.h |  2 +-
>>> 3 files changed, 17 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/doc/APIchanges b/doc/APIchanges
>>> index b5fadc2a48..65b8ed74ad 100644
>>> --- a/doc/APIchanges
>>> +++ b/doc/APIchanges
>>> @@ -15,6 +15,9 @@ libavutil: 2017-10-21
>>> 
>>> API changes, most recent first:
>>> 
>>> +2019-07-XX - XX - lavu 56.31.100 - mem.h
>>> +  Add av_memcpy()
>>> +
>>> 2019-06-21 - XX - lavu 56.30.100 - frame.h
>>>   Add FF_DECODE_ERROR_DECODE_SLICES
>>> 
>>> diff --git a/libavutil/mem.h b/libavutil/mem.h
>>> index 5fb1a02dd9..a35230360d 100644
>>> --- a/libavutil/mem.h
>>> +++ b/libavutil/mem.h
>>> @@ -506,6 +506,19 @@ void *av_memdup(const void *p, size_t size);
>>>  */
>>> void av_memcpy_backptr(uint8_t *dst, int back, int cnt);
>>> 
>>> +/**
>>> + * memcpy() implementation without a NULL pointer special case
>>> + *
>>> + * @param dst  Destination buffer
>>> + * @param src  Source buffer
>>> + * @param cnt  Number of bytes to copy; must be >= 0
>>> + */
>>> +static inline void av_memcpy(void *dst, const void *src, size_t cnt)
>> 
>> How many cases are there in the codebase where cnt can be 0, and dst or
>> src NULL, without it having been checked before calling memcpy? And from
>> those, which would not be from situations where the code should have
>> instead aborted and returned ENOMEM, or EINVAL if either of them are
>> function arguments?
> 
> There are around 2500 occurances of memcpy in the codebase
> To awnser your question it would be needed to review all of them and in many
> cases their surrounding code.
> So that is unlikely to be awnsered by anyone accuratly
> 
> Also iam not sure i understand why you ask or why this would matter
> the suggested function allows to simplify cases where the NULL can
> occur, not where it cannot or should not. That is this is intended for
> the cases where we already have or are adding explicit checks to
> avoid the NULL case.
> 
> i could rename this to av_memcpy_nullsafe which would make it clearer but
> also its more to write and read

I admit I thought that a worthwhile idea originally,
but I have to think back to a long time ago that every function added to our 
"API" has a cost of people having to know about it and how to use it.
And if it's currently only 2 places that would benefit I think James is right 
to ask if it makes sense.
Of course another question might be if it might make sense to replace all 
memcpy uses with it.
I mean, isn't it naturally expected behaviour that the pointers would be 
ignored if the copy amount is 0? There might be a lot of code assuming that we 
do not know about...
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH]lavc/frame_thread_encoder: Do not memcpy() from NULL

[Reimar Döffinger]

On 01.07.2019, at 00:51, Carl Eugen Hoyos  wrote:

> Hi!
> 
> I believe attached patch fixes undefined behaviour and ticket #7981.

Same here, I think it makes more sense to check the "size" instead of the 
pointer.
But I also suspect we might want to think of a way to not need all these 
explicit checks all over.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH]lavf/nutenc: Do not call memcmp() with NULL argument

[Reimar Döffinger]

On 01.07.2019, at 01:12, Carl Eugen Hoyos  wrote:
> Undefined behaviour was reported in ticket #7981, attached patch tries
> to fix it.

I suspect it makes more sense to check header_len against 0?
And is the NULL pointer really undefined behaviour even if length is 0?

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] Changes in cofigure script

[Reimar Döffinger]

On 29.06.2019, at 18:26, Dmitry A  wrote:

> сб, 29 июн. 2019 г. в 21:43, Dmitry A :
> 
>> 
>> 
>> сб, 29 июн. 2019 г. в 19:11, Dmitry A :
>> 
>>> 
>>> 
>>> сб, 29 июн. 2019 г. в 19:04, Carl Eugen Hoyos :
>>> 
 Am Sa., 29. Juni 2019 um 14:23 Uhr schrieb Dmitry A <
 dmitry.adj...@gmail.com>:
 
> What is right way to make changes in the cofigure script?
> I hacked it for building ffmpeg for android since clang doesn't
> support -mcpu and -march or something else.
 
 Please elaborate, building for Android works fine here.
 
 Carl eugen
 ___
 ffmpeg-devel mailing list
 ffmpeg-devel@ffmpeg.org
 https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
 
 To unsubscribe, visit link above, or email
 ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
>>> 
>>> 
>>> I integrated ffmpeg in aosp inside for one micro chip. I created simple
>>> library  for implementation
>>> fake camera based on video file etc.
>>> But I built the latest ffmpeg with latest NDK. Please share the options
>>> which you use.
>>> 
>> --
>>> Thanks!
>>> Dmitry
>>> 
>> 
>> One more question: did you build it for android with NDK with clang?
>> If so please share your configure options.
>> Also I remember some errors related with vairable names such as B0.
>> I'll share the output later.
>> --
>> Thanks!
>> Dmitry
>> 
> 
> That's what I told:
> 
> BEGIN /tmp/ffconf.MukWI5j9/test.c
>1 int main(void){ return 0; }
> END /tmp/ffconf.MukWI5j9/test.c
> /d/Android/android-sdk/ndk-bundle/toolchains/llvm/prebuilt/windows-x86_64/bin/aarch64-linux-androideabi-gcc
> --sysroot=/d/Android/android-sdk/ndk-bundle/platforms/android-29/arch-arm64/
> -Os -fpic -mcpu= -c -o /tmp/ffconf.MukWI5j9/test.o
> /tmp/ffconf.MukWI5j9/test.c
> clang: warning: joined argument expects additional value: '-mcpu='
> [-Wunused-command-line-argument]

That is the real error.
the mcpu argument is empty, which makes no sense.
You need to find out why/how that happens.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v6] Fix integer parameters size check in SDP fmtp line

[Reimar Döffinger]

I don't think we should be using errno when avoidable, and it is avoidable here 
by disallowing min/max int32 values themselves. Or using strtoll.
I'm also rather sceptical about allowing negative values here, does that make 
sense?
Admittedly the type is set to just "int", but maybe it should be unsigned 
instead?

On 28.06.2019, at 08:46, Olivier MAIGNIAL  wrote:

> Hello here!
> 
> A simple ping about this patch
> If you have any question, feel free to ask!
> 
> Regards,
> Olivier
> 
> On Wed, Jun 19, 2019 at 3:38 PM Olivier Maignial 
> wrote:
> 
>> === PROBLEM ===
>> 
>> I was trying to record h264 + aac streams from an RTSP server to mp4 file.
>> using this command line:
>>ffmpeg -v verbose -y -i "rtsp:///my_resources" -codec copy -bsf:a
>> aac_adtstoasc test.mp4
>> 
>> FFmpeg then fail to record audio and output this logs:
>>[rtsp @ 0xcda1f0] The profile-level-id field size is invalid (40)
>>[rtsp @ 0xcda1f0] Error parsing AU headers
>>...
>>[rtsp @ 0xcda1f0] Could not find codec parameters for stream 1 (Audio:
>> aac, 48000 Hz, 1 channels): unspecified sample format
>> 
>> In SDP provided by my RTSP server I had this fmtp line:
>>a=fmtp:98 streamType=5; profile-level-id=40; mode=AAC-hbr;
>> config=1188; sizeLength=13; indexLength=3; indexDeltaLength=3;
>> 
>> In FFmpeg code, I found a check introduced by commit
>> 24130234cd9dd733116d17b724ea4c8e12ce097a. It disallows values greater than
>> 32 for fmtp line parameters.
>> However, In RFC-6416 (RTP Payload Format for MPEG-4 Audio/Visual Streams)
>> give examples of "profile-level-id" values for AAC, up to 55.
>> Furthermore, RFC-4566 (SDP: Session Description Protocol) do not give any
>> limit of size on interger parameters given in fmtp line.
>> 
>> === FIX ===
>> 
>> Instead of prohibit values over 32, I propose to check the possible
>> integer overflow.
>> The use of strtol allow to check the string validity and the possible
>> overflow.
>> Value is then checked against INT32_MIN and INT32_MAX. Using INT32_MIN/MAX
>> ensure to have the same behavior on 32 or 64 bits platforms.
>> 
>> This patch fix my problem and I now can record my RTSP AAC stream to mp4.
>> It has passed the full fate tests suite sucessfully.
>> 
>> Signed-off-by: Olivier Maignial 
>> ---
>> 
>> Changes V5 -> V6:
>>- Simplify code
>> 
>> libavformat/rtpdec_mpeg4.c | 15 ++-
>> 1 file changed, 10 insertions(+), 5 deletions(-)
>> 
>> diff --git a/libavformat/rtpdec_mpeg4.c b/libavformat/rtpdec_mpeg4.c
>> index 4f70599..9c4f8a1 100644
>> --- a/libavformat/rtpdec_mpeg4.c
>> +++ b/libavformat/rtpdec_mpeg4.c
>> @@ -289,15 +289,20 @@ static int parse_fmtp(AVFormatContext *s,
>> for (i = 0; attr_names[i].str; ++i) {
>> if (!av_strcasecmp(attr, attr_names[i].str)) {
>> if (attr_names[i].type == ATTR_NAME_TYPE_INT) {
>> -int val = atoi(value);
>> -if (val > 32) {
>> +char *end_ptr = NULL;
>> +errno = 0;
>> +long int val = strtol(value, _ptr, 10);
>> +if (end_ptr == value || end_ptr[0] != '\0' ||
>> +errno == ERANGE ||
>> +val < INT32_MIN || val > INT32_MAX) {
>> av_log(s, AV_LOG_ERROR,
>> -   "The %s field size is invalid (%d)\n",
>> -   attr, val);
>> +   "The %s field value is not a valid number,
>> or overflows int32: %s\n",
>> +   attr, value);
>> return AVERROR_INVALIDDATA;
>> }
>> +
>> *(int *)((char *)data+
>> -attr_names[i].offset) = val;
>> +attr_names[i].offset) = (int) val;
>> } else if (attr_names[i].type == ATTR_NAME_TYPE_STR) {
>> char *val = av_strdup(value);
>> if (!val)
>> --
>> 2.7.4
>> 
>> 
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/4] avformat/vividas: Fixes overflow in shift in recover_key()

[Reimar Döffinger]

On 28.06.2019, at 22:53, Michael Niedermayer  wrote:

> Fixes: left shift of 133 by 24 places cannot be represented in type 'int'
> Fixes: 
> 15365/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5716153105645568
> 
> Found-by: continuous fuzzing process 
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer 
> ---
> libavformat/vividas.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavformat/vividas.c b/libavformat/vividas.c
> index 753328245d..ed2eaea633 100644
> --- a/libavformat/vividas.c
> +++ b/libavformat/vividas.c
> @@ -118,7 +118,7 @@ static unsigned recover_key(unsigned char sample[4], 
> unsigned expected_size)
> return (sample[0]^plaintext[0])|
> ((sample[1]^plaintext[1])<<8)|
> ((sample[2]^plaintext[2])<<16)|
> -((sample[3]^plaintext[3])<<24);
> +((unsigned)(sample[3]^plaintext[3])<<24);

Shouldn't this just be
return AV_RL32(sample) ^ AV_RL32(plaintext);
?
If so, the code might be worthy of review for more needless overcomplication...
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/3] avcodec/cfhd: remove unused function

[Reimar Döffinger]

On 27.06.2019, at 17:35, Vittorio Giovara  wrote:

> On Thu, Jun 27, 2019 at 9:44 AM Nicolas George  wrote:
> 
>> Kieran Kunhya (12019-06-27):
>>> I'm happy to do it now that I am aware of the issue. I will do it when I
>> am
>>> at home in a few days.
>> 
>> Thanks. I am sure Steven will not mind waiting a few days.
>> 
>>> This absolutism is absurd.
>> 
>> Do you have an example of situation where dead code is good?
>> 
> 
> If i could add my 2 cents, for a reverse engineered codec it's important to
> leave unused functions purely for documentation purposes, so that future
> maintainers could implement and read about it right away, rather than
> digging in a large and messy git history.
> Additionally most compilers run passes that drop dead code already in a way
> that does not affect runtime one bit. So I really don't see any gains in
> removing these 14 lines of code.

I'd note that intentionally dead code should at least have a comment, and
maybe even a #if 0 would make sense (though has the disadvantage of not
even compile-testing the code).
In case of an actual bug like here I would say dead code if nothing else is a 
reminder of the
bug, though admittedly a very poor one.
Either way I suggest to discuss such things more relaxed, a few days more or
less hardly matters and there might be useful insights from other people (of
course I don't mean to delay non-controversial stuff nobody has any 
comments/objections on).

Best regards,
Reimar Döffinger
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avcodec: add delayer bitstream filter

[Reimar Döffinger]

On 26.06.2019, at 13:15, Andreas Håkon  wrote:

> Hi Moritz,
> 
>>> Wouldn't this be better if extended to be a BSF version of setpts? In
>>> addition to delays, rescaling as well as other ops could be carried out
>>> on TS.
>> 
>> I second that notion, or at least the suggestion of a setpts bsf.
>> 
>> IMO, there is probably some functionality in filters which could be
>> implemented in bitstream filters, to allow it to apply to stream copy.
> 
> I think it's a good idea.
> 
> Just to say that from my point of view, it would be easier if another
> developer took care of including the functionality of other filters,
> because my knowledge of the project is not so deep.
> Still, I feel that it would be best to incorporate this BSF as it is
> and later on add more features.

Minor note: I'd suggest a different name, "delayer" is too easily read as e.g. 
"de-layer".
Could be something like add_delay for example.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH V1] lavfi/normalize: improve the performance

[Reimar Döffinger]

On Thu, Jun 20, 2019 at 09:43:46PM +0800, Jun Zhao wrote:
> From: Jun Zhao 
>
> Remove unnecessary max value found, it's will improve the performance
> about 10%. Used the test command like:
> ffmpeg -i 1080P.mp4 -an -vf normalize -f null /dev/null, the FPS change
> from 96fps to 107fps.
>
> Signed-off-by: Jun Zhao 
> ---
>  libavfilter/vf_normalize.c |7 +++
>  1 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/libavfilter/vf_normalize.c b/libavfilter/vf_normalize.c
> index 48eea59..40dc031 100644
> --- a/libavfilter/vf_normalize.c
> +++ b/libavfilter/vf_normalize.c
> @@ -143,14 +143,13 @@ static void normalize(NormalizeContext *s, AVFrame *in, 
> AVFrame *out)
>  min[c].in = max[c].in = in->data[0][s->co[c]];
>  for (y = 0; y < in->height; y++) {
>  uint8_t *inp = in->data[0] + y * in->linesize[0];
> -uint8_t *outp = out->data[0] + y * out->linesize[0];
>  for (x = 0; x < in->width; x++) {
>  for (c = 0; c < 3; c++) {
> -min[c].in = FFMIN(min[c].in, inp[s->co[c]]);
> -max[c].in = FFMAX(max[c].in, inp[s->co[c]]);
> +uint8_t val = inp[s->co[c]];
> +if (val < min[c].in)  min[c].in = val;
> +else if (val > max[c].in) max[c].in = val;
>  }
>  inp += s->step;
> -outp += s->step;

outp should just be removed, a patch for that certainly would be
accepted.
But I am very skeptical towards the rest of the change.
In a way I would expect the compiler should be able to
optimize this.
However this code is clearly completely unoptimized, and
thus I don't think making it more complex for a minor optimization
that gives just 10% is worth it.
A proper optimization would get rid of the s->co indirection in the
inner loop - it really is not necessary for the limited format
support the code has.
This would then also allow for SIMD optimization, which should give
a vastly larger speedup - and possibly the compiler would even be able
to auto-vectorize.
This applies even more to the processing step - the transform
calculation done in SIMD is almost certain to be cheaper than
a LUT lookup - at least if switch to fixed-point arithmetic
instead of float.
Side note: I haven't really figured out what the point
of all this s->co complexity is supposed to be.
The only thing the code seems to actually care is whether
there is an alpha channel, and if so where it is.
But the quickest path to a significant speedup would
be to write a SIMD version of the normalize function specific
to that format and the SIMD instruction set available
and call it instead of normalize if applicable.
I would expect that you should see at least a 3x speedup
if using e.g. SSE2 (which is why I don't consider a
10% speedup worth changing the code for really).

Best regards,
Reimar Döffinger
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] avcodec/bink: Fix integer overflow in unquantize_dct_coeffs()

[Reimar Döffinger]

On 18.06.2019, at 14:55, Michael Niedermayer  wrote:

> Fixes: signed integer overflow: -3447 * 2883584 cannot be represented in type 
> 'int'
> Fixes: 
> 15265/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_BINK_fuzzer-5088311799971840
> 
> Found-by: continuous fuzzing process 
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer 
> ---
> libavcodec/bink.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/bink.c b/libavcodec/bink.c
> index 8392bbeeb0..d18c0ceae4 100644
> --- a/libavcodec/bink.c
> +++ b/libavcodec/bink.c
> @@ -702,15 +702,15 @@ static int read_dct_coeffs(BinkContext *c, 
> GetBitContext *gb, int32_t block[64],
> return quant_idx;
> }
> 
> -static void unquantize_dct_coeffs(int32_t block[64], const int32_t quant[64],
> +static void unquantize_dct_coeffs(int32_t block[64], const uint32_t 
> quant[64],
>   int coef_count, int coef_idx[64],
>   const uint8_t *scan)
> {
> int i;
> -block[0] = (block[0] * quant[0]) >> 11;
> +block[0] = (int)(block[0] * quant[0]) >> 11;

Huh? How do you know the multiplication result will fit in an int?
IIRC casting an out-of-range value to int is undefined behaviour, or does the 
tool fail to check that?
I might miss something, but it looks to me like just replacing one undefined 
behaviour with another...
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avcodec/videodsp_template: Fix overflow of addition

[Reimar Döffinger]

On 18.06.2019, at 16:25, Michael Niedermayer  wrote:

> Fixes: addition of unsigned offset to 0x7f56fc26a9b6 overflowed to 
> 0x7f56fc26a8be*
> Fixes: 
> clusterfuzz-testcase-minimized-mediasource_MP4_AVC1_pipeline_integration_fuzzer-4917949056679936
> 
> Reported-by: Matt Wolenetz 
> Reviewed-by: Matt Wolenetz 
> Signed-off-by: Michael Niedermayer 
> ---
> libavcodec/videodsp_template.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavcodec/videodsp_template.c b/libavcodec/videodsp_template.c
> index 94c1b7188d..eae2f1d51b 100644
> --- a/libavcodec/videodsp_template.c
> +++ b/libavcodec/videodsp_template.c
> @@ -44,7 +44,7 @@ void FUNC(ff_emulated_edge_mc)(uint8_t *buf, const uint8_t 
> *src,
> src_y = 1 - block_h;
> }
> if (src_x >= w) {
> -src  += (w - 1 - src_x) * sizeof(pixel);
> +src  -= (1 + src_x - w) * sizeof(pixel);

This is really non-obvious and someone might be tempted to "simplify", 
especially since the old way matched the code a few lines below.
I'd suggest adding a comment.

> src_x = w - 1;
> } else if (src_x <= -block_w) {
> src  += (1 - block_w - src_x) * sizeof(pixel);
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 3/7] avcodec/alsdec: Fix integer overflow with buffer number

[Reimar Döffinger]

On 21.06.2019, at 00:47, Michael Niedermayer  wrote:

> Fixes: signed integer overflow: 65313 * 65313 cannot be represented in type 
> 'int'
> Fixes: 
> 15290/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ALS_fuzzer-5738074249625600
> 
> Found-by: continuous fuzzing process 
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer 
> ---
> libavcodec/alsdec.c | 2 ++
> 1 file changed, 2 insertions(+)
> 
> diff --git a/libavcodec/alsdec.c b/libavcodec/alsdec.c
> index 79d22b7c2b..8e0d3e5f83 100644
> --- a/libavcodec/alsdec.c
> +++ b/libavcodec/alsdec.c
> @@ -1990,6 +1990,8 @@ static av_cold int decode_init(AVCodecContext *avctx)
> 
> // allocate quantized parcor coefficient buffer
> num_buffers = sconf->mc_coding ? avctx->channels : 1;
> +if (num_buffers * (uint64_t)num_buffers > INT_MAX)
> +return AVERROR_INVALIDDATA;

It would be nice if it was clear which code this check protects, i.e. some 
connection between the check and the code that overflows.
I guess one might also ask if > 30 000 channels might not be something to catch 
and disallow earlier and generally...
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/4] avcodec/apedec: Fix multiple integer overflows in filter_3800()

[Reimar Döffinger]

On 16.06.2019, at 17:12, Paul B Mahol  wrote:

> On 6/16/19, Reimar Döffinger  wrote:
>> 
>> 
>> On 16.06.2019, at 12:30, Lynne  wrote:
>> 
>>> Jun 16, 2019, 10:57 AM by mich...@niedermayer.cc:
>>> 
>>>> Fixes: left shift of negative value -4
>>>> Fixes: signed integer overflow: -15091694 * 167 cannot be represented in
>>>> type 'int'
>>>> Fixes: signed integer overflow: 1898547155 + 453967445 cannot be
>>>> represented in type 'int'
>>>> Fixes:
>>>> 15258/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_APE_fuzzer-5759095564402688
>>>> 
>>>> Found-by: continuous fuzzing process
>>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>> Signed-off-by: Michael Niedermayer 
>>>> ---
>>>> libavcodec/apedec.c | 16 
>>>> 1 file changed, 8 insertions(+), 8 deletions(-)
>>>> 
>>>> diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c
>>>> index 61ebfdafd5..f1bfc634c2 100644
>>>> --- a/libavcodec/apedec.c
>>>> +++ b/libavcodec/apedec.c
>>>> @@ -859,22 +859,22 @@ static av_always_inline int
>>>> filter_3800(APEPredictor *p,
>>>> return predictionA;
>>>> }
>>>> d2 =  p->buf[delayA];
>>>> -d1 = (p->buf[delayA] - p->buf[delayA - 1]) << 1;
>>>> -d0 =  p->buf[delayA] + ((p->buf[delayA - 2] - p->buf[delayA - 1]) <<
>>>> 3);
>>>> +d1 = (p->buf[delayA] - p->buf[delayA - 1]) * 2;
>>>> +d0 =  p->buf[delayA] + ((p->buf[delayA - 2] - p->buf[delayA - 1]) *
>>>> 8);
>>>> d3 =  p->buf[delayB] * 2 - p->buf[delayB - 1];
>>>> d4 =  p->buf[delayB];
>>>> 
>>>> -predictionA = d0 * p->coeffsA[filter][0] +
>>>> -  d1 * p->coeffsA[filter][1] +
>>>> -  d2 * p->coeffsA[filter][2];
>>>> +predictionA = d0 * (unsigned)p->coeffsA[filter][0] +
>>>> +  d1 * (unsigned)p->coeffsA[filter][1] +
>>>> +  d2 * (unsigned)p->coeffsA[filter][2];
>>>> 
>>>> sign = APESIGN(decoded);
>>>> p->coeffsA[filter][0] += (((d0 >> 30) & 2) - 1) * sign;
>>>> p->coeffsA[filter][1] += (((d1 >> 28) & 8) - 4) * sign;
>>>> p->coeffsA[filter][2] += (((d2 >> 28) & 8) - 4) * sign;
>>>> 
>>>> -predictionB = d3 * p->coeffsB[filter][0] -
>>>> -  d4 * p->coeffsB[filter][1];
>>>> +predictionB = d3 * (unsigned)p->coeffsB[filter][0] -
>>>> +  d4 * (unsigned)p->coeffsB[filter][1];
>>>> p->lastA[filter] = decoded + (predictionA >> 11);
>>>> sign = APESIGN(p->lastA[filter]);
>>>> p->coeffsB[filter][0] += (((d3 >> 29) & 4) - 2) * sign;
>>>> @@ -902,7 +902,7 @@ static void long_filter_high_3800(int32_t *buffer,
>>>> int order, int shift, int len
>>>> dotprod = 0;
>>>> sign = APESIGN(buffer[i]);
>>>> for (j = 0; j < order; j++) {
>>>> -dotprod += delay[j] * coeffs[j];
>>>> +dotprod += delay[j] * (unsigned)coeffs[j];
>>>> coeffs[j] += ((delay[j] >> 31) | 1) * sign;
>>>> }
>>>> buffer[i] -= dotprod >> shift;
>>>> 
>>> 
>>> This code is DSP, overflows should be allowed in case input is corrupt.
>> 
>> Then get the C standard changed or use a different language.
>> But in C overflows on signed types absolutely are not Ok.
> 
> I disagree, overflows in DSP are normal.

It's simply incorrect code.
Yes, you will probably get lucky for a long time.
But there are ISAs that trap on overflow, and gcc can trap on overflow.
That already makes these cases a denial-of-service issue.
However it gets far worse than that.
All it takes is for code to do something like
if (x < 2048) some_array[x] = y;
(lots of code followed by DSP code doing)
x * (1 << 24)

and now the compiler can and actually might really remove the if condition and 
you have an exploitable buffer overflow.
People have been bitten by such issues often enough (because due to LTO 
compiler optimizations of this kind have potentially unlimited scope, whereas 
reviewer have almost not chance of catching something this subtle) that such 
assumptions violating the C standard should be avoided.
(besides the more immediate point that fuzzing does catch quite real issues, 
and leaving such well-maybe-it's-ok cases in prevents finding real issues - on 
its own it might be an argument to change to tools, but due to the above it is 
reasonable to consider the tools doing the right thing).
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/4] avcodec/apedec: Fix multiple integer overflows in filter_3800()

[Reimar Döffinger]

On 16.06.2019, at 12:30, Lynne  wrote:

> Jun 16, 2019, 10:57 AM by mich...@niedermayer.cc:
> 
>> Fixes: left shift of negative value -4
>> Fixes: signed integer overflow: -15091694 * 167 cannot be represented in 
>> type 'int'
>> Fixes: signed integer overflow: 1898547155 + 453967445 cannot be represented 
>> in type 'int'
>> Fixes: 
>> 15258/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_APE_fuzzer-5759095564402688
>> 
>> Found-by: continuous fuzzing process 
>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> Signed-off-by: Michael Niedermayer 
>> ---
>> libavcodec/apedec.c | 16 
>> 1 file changed, 8 insertions(+), 8 deletions(-)
>> 
>> diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c
>> index 61ebfdafd5..f1bfc634c2 100644
>> --- a/libavcodec/apedec.c
>> +++ b/libavcodec/apedec.c
>> @@ -859,22 +859,22 @@ static av_always_inline int filter_3800(APEPredictor 
>> *p,
>> return predictionA;
>> }
>> d2 =  p->buf[delayA];
>> -d1 = (p->buf[delayA] - p->buf[delayA - 1]) << 1;
>> -d0 =  p->buf[delayA] + ((p->buf[delayA - 2] - p->buf[delayA - 1]) << 3);
>> +d1 = (p->buf[delayA] - p->buf[delayA - 1]) * 2;
>> +d0 =  p->buf[delayA] + ((p->buf[delayA - 2] - p->buf[delayA - 1]) * 8);
>> d3 =  p->buf[delayB] * 2 - p->buf[delayB - 1];
>> d4 =  p->buf[delayB];
>> 
>> -predictionA = d0 * p->coeffsA[filter][0] +
>> -  d1 * p->coeffsA[filter][1] +
>> -  d2 * p->coeffsA[filter][2];
>> +predictionA = d0 * (unsigned)p->coeffsA[filter][0] +
>> +  d1 * (unsigned)p->coeffsA[filter][1] +
>> +  d2 * (unsigned)p->coeffsA[filter][2];
>> 
>> sign = APESIGN(decoded);
>> p->coeffsA[filter][0] += (((d0 >> 30) & 2) - 1) * sign;
>> p->coeffsA[filter][1] += (((d1 >> 28) & 8) - 4) * sign;
>> p->coeffsA[filter][2] += (((d2 >> 28) & 8) - 4) * sign;
>> 
>> -predictionB = d3 * p->coeffsB[filter][0] -
>> -  d4 * p->coeffsB[filter][1];
>> +predictionB = d3 * (unsigned)p->coeffsB[filter][0] -
>> +  d4 * (unsigned)p->coeffsB[filter][1];
>> p->lastA[filter] = decoded + (predictionA >> 11);
>> sign = APESIGN(p->lastA[filter]);
>> p->coeffsB[filter][0] += (((d3 >> 29) & 4) - 2) * sign;
>> @@ -902,7 +902,7 @@ static void long_filter_high_3800(int32_t *buffer, int 
>> order, int shift, int len
>> dotprod = 0;
>> sign = APESIGN(buffer[i]);
>> for (j = 0; j < order; j++) {
>> -dotprod += delay[j] * coeffs[j];
>> +dotprod += delay[j] * (unsigned)coeffs[j];
>> coeffs[j] += ((delay[j] >> 31) | 1) * sign;
>> }
>> buffer[i] -= dotprod >> shift;
>> 
> 
> This code is DSP, overflows should be allowed in case input is corrupt.

Then get the C standard changed or use a different language.
But in C overflows on signed types absolutely are not Ok.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avformat/oggparseogm: unknown codec triggers error

[Reimar Döffinger]

On 14.06.2019, at 17:01, James Almer  wrote:

> On 6/14/2019 11:52 AM, Reimar Döffinger wrote:
>> 
>> 
>> On 14.06.2019, at 03:15, Chris Cunningham  wrote:
>> 
>>> Only "succeed" to read a header if the codec is valid. Otherwise
>>> return AVERROR_INVALIDDATA.
>> 
>> That doesn't sound right to me, an unknown codec in (possibly) a single 
>> stream is not an error.
>> I understood the discussion more to say the if it's an unknown codec, we 
>> should not try to override valid codec configuration with a broken one.
> 
> I did request this change, seeing that returning codec_id none in this
> scenario results in a crash at a later point due to conflicting parameters.
> 
> Do you suggest we should limit the change to only reject any duplicate
> header that may show up after the first one (and before the first data
> packet)?

I don't know or understand the details, but I understood the suggestion as "do 
not override a valid codec ID to NONE".
Either way, I would have suggested only skipping the affected stream - but I 
admit I have not checked how far-reaching it is to return an error here.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".