[FFmpeg-devel] [PATCH] avcodec/ffv1enc: further reduce stack usage

[James Almer]

On 3/24/2025 10:50 PM, Michael Niedermayer wrote:
> Hi
> 
> On Mon, Mar 24, 2025 at 07:20:50PM -0300, James Almer wrote:
>> Continues from commit 702239bc500b, fixing FATE failures on MacOS.
>>
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>> Confirmed by Martin Storsjö. Float encoding untested.
>>
>>   libavcodec/ffv1.h    |  16 ++++
>>   libavcodec/ffv1enc.c | 177 +++++++++++++++++--------------------------
>>   2 files changed, 84 insertions(+), 109 deletions(-)
>>
>> diff --git a/libavcodec/ffv1.h b/libavcodec/ffv1.h
>> index 09118e0b7d..d1c239f138 100644
>> --- a/libavcodec/ffv1.h
>> +++ b/libavcodec/ffv1.h
>> @@ -115,6 +115,22 @@ typedef struct FFV1SliceContext {
>>           uint32_t val; //this is unneeded if you accept a dereference on each access
>>           uint16_t ndx;
>>       } unit[4][65536];
>> +    struct RemapEncoderState {
>> +        int delta_stack[65536];     //We need to encode the run value before the adjustments, this stores the adjustments until we know the length of the run
>> +        int16_t index_stack[65537]; //only needed with multiple segments
>> +        uint8_t state[2][3][32];
>> +        int mul[4096+1];
>> +        RangeCoder rc;
>> +        int lu;
>> +        int run;
>> +        int64_t last_val;
>> +        int compact_index;
>> +        int mul_count;
>> +        int i;
>> +        int pixel_num;
>> +        int p;
>> +        int current_mul_index;
>> +    } remap_state;
>>   } FFV1SliceContext;
> 
> please provide a link to the failure

Martin will have to do that. I can't seem to find any FATE instance 
failing, but he said it affected his OSX machines.

> 
> This makes the code increasingly ugly.
> 
> i dont understand why this breaks fate, fate should not use
> any of the float code as none should be run in fate ATM.
> its also all under -strict -2 checks

It also surprised me, since these are functions that need to be called, 
unlike the fix in 702239bc500b which was in a function actually called 
by existing tests.

> 
> this is temporary data not needed outside float32
> and not needed outside the remap table writing.
> 
> we may need more than one such state.
> (if we dont use a heuristic but actually
>   encode bruteforce / trial and error)
> 
> t conflicts with all work i did today
> 
> theres tons of unused memory.
> 
> We ATM do 2 things in encode_float32_remap_segment()
> one is encoding the table
> the other is writing the remaped pixels into sc->bitmap
> by using unit[s.p][s.i].ndx
> 
> sc->bitmap is unused before, unit[s.p][s.i].ndx unused afterwards
> the input image itself is also not used again
> half of fltmap32 is unused (thats 512kb alone here)

Yeah, ideally all this is allocated only when needed rather than 
unconditionally in the slice context. But i didn't go that far since i 
can't even reproduce this issue.

> 
> the code can be writen so it doesnt need the stack
> but just runs twice over the stuff (not sure how clean this
> would be but if you try _please_ do it on top of the patches
> i posted today, the code is simpler and less buggy after
> these patches
> 
> But i really dont understand why fate fails in relation to code
> it never executes.

For the issue fixed in 702239bc500b, i guess it did attempt to reserve 
stack space even if it never used it. For this one? Beats me.

> 
> thx
> 
> [...]
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 495 bytes
Desc: OpenPGP digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20250324/fe868f11/attachment.sig>