VSE: Cleanup: Remove storage of last FFMPEG frame read as ImBuf in sequences. #114395

Bastien Montagne · 2023-11-02T10:49:13+01:00

Bastien Montagne commented

2023-11-02 10:49:13 +01:00

The anim data of a sequence would store the last frame read from
FFMPEG as an image buffer, increasing its refcounting and preventing it
to be freed until the whole anim data itself gets freed.

In current code, there is no reason to keep a reference to this image
buffer in the anim data, so removing it. This may also give a few
percent improvement on the memory usage of the VSE in some cases.

Found while investigating #114342.

The `anim` data of a sequence would store the last frame read from FFMPEG as an image buffer, increasing its refcounting and preventing it to be freed until the whole `anim` data itself gets freed. In current code, there is no reason to keep a reference to this image buffer in the `anim` data, so removing it. This may also give a few percent improvement on the memory usage of the VSE in some cases. Found while investigating #114342.

Bastien Montagne requested review from Sergey Sharybin 2023-11-02 10:49:43 +01:00

Bastien Montagne requested review from Richard Antalik 2023-11-02 10:49:43 +01:00

Bastien Montagne commented

2023-11-02 10:49:53 +01:00

@blender-bot build

Sergey Sharybin approved these changes 2023-11-02 11:39:24 +01:00

Richard Antalik commented

2023-11-02 18:25:12 +01:00

Just a note, that this change is not VSE specific, but applies to the imbuf module which is used by bunch of areas. I like to prefix title of such commits with "FFmpeg:".

I think at some point there was path in code, where cur_frame_final was returned early to avoid re-processing same AVFrame. I did a quick test with 1 FPS movie converted to 10 FPS, and indeed same AVFrame was processed each time into ImBuf. Question is, whether it is good idea to keep this buffer around and do early return. This could save some performance and memory when using for example 30fps movie in 60fps scene. This would apply mostly to VSE since it does support time remapping. Other editors may use scene time stretching feature, but not sure if that would yield the same effect and it probably isn't used often.

Drawback of this would be bit increased code complexity - pointer to previous frame would have to be stored and compared to final_frame in ffmpeg_fetchibuf(). Perhaps nicer could be wrapping cur_frame_final, so it is aware of what AVFrame it belongs to.

That said, early return has not been there for some time, so I think the cur_frame_final as it is now could be removed. A form of caching could be implemented again in a way that is more obvious and does not pollute anim structure with data not related to core functionality.

Just a note, that this change is not VSE specific, but applies to the imbuf module which is used by bunch of areas. I like to prefix title of such commits with "FFmpeg:". I think at some point there was path in code, where `cur_frame_final` was returned early to avoid re-processing same `AVFrame`. I did a quick test with 1 FPS movie converted to 10 FPS, and indeed same `AVFrame` was processed each time into `ImBuf`. Question is, whether it is good idea to keep this buffer around and do early return. This could save some performance and memory when using for example 30fps movie in 60fps scene. This would apply mostly to VSE since it does support time remapping. Other editors may use scene time stretching feature, but not sure if that would yield the same effect and it probably isn't used often. Drawback of this would be bit increased code complexity - pointer to previous frame would have to be stored and compared to `final_frame` in `ffmpeg_fetchibuf()`. Perhaps nicer could be wrapping `cur_frame_final`, so it is aware of what `AVFrame` it belongs to. That said, early return has not been there for some time, so I think the `cur_frame_final` as it is now could be removed. A form of caching could be implemented again in a way that is more obvious and does not pollute `anim` structure with data not related to core functionality.

👍 1

Richard Antalik approved these changes 2023-11-02 18:25:26 +01:00

Bastien Montagne force-pushed tmp-vse-anim-remove-cur-ibuf from 45270513cb to b1d2198aad

2023-11-03 11:08:24 +01:00

Compare

Bastien Montagne merged commit d1e732a114 into main

2023-11-03 11:18:30 +01:00

Bastien Montagne referenced this issue from a commit

2023-11-03 11:18:31 +01:00

FFMPEG: Cleanup: Remove storage of last FFMPEG frame read as `ImBuf` in anim data.

Bastien Montagne deleted branch tmp-vse-anim-remove-cur-ibuf

2023-11-03 11:18:32 +01:00

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

VSE: Cleanup: Remove storage of last FFMPEG frame read as ImBuf in sequences. #114395

VSE: Cleanup: Remove storage of last FFMPEG frame read as `ImBuf` in sequences. #114395