[FFmpeg-devel] [PATCH] avcodec/ffv1enc: further reduce stack usage
James Almer
jamrial at gmail.com
Tue Mar 25 03:57:28 EET 2025
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>
More information about the ffmpeg-devel
mailing list