VSE: Reduce playback stalls when new video clips start playing #118503

Aras Pranckevicius · 2024-02-20T13:58:03+01:00

2024-02-20 13:58:03 +01:00

When starting to play back a movie strip, there was a "oh, let's figure out if this is a video file?" check, immediately followed by "let's initialize the machinery to decode a video file". The first one is kinda redundant, since if it will happen to not be a video file, the latter
check will nicely fail.

Addressed that by removing (seemingly unused at all?) functionality from ImBufAnim, now it is only used for video file playback. Details:

It looks like ImBufAnim is only ever used to play back "video files", so remove all the other modes of operation from it ("image sequence").
Which makes ImBufAnim::curtype be not needed, it only needs to store state of whether it's initialized already.
Which means there's no need to call imb_get_anim_type (which does very costly isffmpeg) when starting to play ImBufAnim.
Remove some other variables from ImBufAnim that were just flat out unused.

In Gold previs playback between 1:41-1:55, on Windows/Ryzen5950X:

Slowest 3 frames went from 276, 195, 168ms -> 222, 174, 147ms (saves 20+ ms per frame). All of these frames are camera cuts where multiple new video clips start playing.
In the whole playback, total amount of time taken by imb_get_anim_type: 234ms -> zero! Since that is no longer used.
There are still stalls when starting to play a movie clip, and that is actually initializing ffmpeg things. But now at least right before "initialize ffmpeg" there's no additional redundant work.

When starting to play back a movie strip, there was a "oh, let's figure out if this is a video file?" check, immediately followed by "let's initialize the machinery to decode a video file". The first one is kinda redundant, since if it will happen to not be a video file, the latter check will nicely fail. Addressed that by removing (seemingly unused at all?) functionality from `ImBufAnim`, now it is only used for video file playback. Details: - It looks like `ImBufAnim` is only ever used to play back "video files", so remove all the other modes of operation from it ("image sequence"). - Which makes `ImBufAnim::curtype` be not needed, it only needs to store state of whether it's initialized already. - Which means there's no need to call `imb_get_anim_type` (which does very costly `isffmpeg`) when starting to play ImBufAnim. - Remove some other variables from `ImBufAnim` that were just flat out unused. In Gold previs playback between 1:41-1:55, on Windows/Ryzen5950X: - Slowest 3 frames went from 276, 195, 168ms -> 222, 174, 147ms (saves 20+ ms per frame). All of these frames are camera cuts where multiple new video clips start playing. - In the whole playback, total amount of time taken by `imb_get_anim_type`: 234ms -> zero! Since that is no longer used. - There are still stalls when starting to play a movie clip, and that is actually initializing ffmpeg things. But now at least right before "initialize ffmpeg" there's no additional redundant work.

Aras Pranckevicius added 2 commits 2024-02-20 13:58:12 +01:00

18fef523ee Cleanup: Video: remove no-op ismovie and friends

ismovie/startmovie/movie_fetchibuf/free_anim_movie were doing nothing.

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-lint Build done. Details

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-coordinator Build done. Details

db9aeff87b VSE: Speedup isffmpeg by doing a simpler check

isffmpeg is being used to answer the question, "is this a video file?".
It is generally called already on files that are known to have video
extensions (e.g. .mp4), and was doing a full-blown "initialize
libavformat decoding context, read header, decode some frames just to
be sure, find video stream, see if we have a suitable video stream
decoder".

Now changed that to only do the "initialize decoding context, read
header" and if that is successful, assume it is a file that ffmpeg
can understand. In theory this can change output from "false"
previously to "true" now, for files that are corrupted or have no video
streams. In practice, all that is moot since the later "try to playback"
code will do all that checking anyway.

In tests of VSE with broken / no-video files, end-user result looks
exactly the same, e.g. dragging a no-video file into VSE says
"not an anim" just like before.

In Gold previs playback between 1:41-1:55, on Windows/Ryzen5950X:
- Slowest 3 frames went from 276, 195, 168ms -> 239, 173, 147ms
  (saves ~20ms per frame). All of these frames are camera cuts where
  multiple new video clips start playing.
- In the whole playback, total amount of time taken by
  `imb_get_anim_type`: 234ms -> 42ms.

Aras Pranckevicius added this to the Video Sequencer project 2024-02-20 14:05:49 +01:00

Aras Pranckevicius requested review from Sergey Sharybin 2024-02-20 14:13:44 +01:00

Sergey Sharybin commented

2024-02-20 14:32:48 +01:00

It seems that this patch will interfere with the code which tries to tell video from audio .ogg files in ED_path_extension_type().

There are few things which I am not sure why we need to have/do:

Do we really need to run the detection every time we start playing the strip? Intuitively, you look into the file when you drop it, create Video, Audio, or Image strip, but the specific strip type tries to interpret the file according to the strip type. This means, that, say, Video strip will always consider the file being a video, and use FFmpeg.
Why do we need ANIM_SEQUENCE type of ImBufAnim? Do we ever use ImBufAnim to access image sequences? I thought we use ImBuf for it, not the anim API.

It seems that this patch will interfere with the code which tries to tell video from audio `.ogg` files in `ED_path_extension_type()`. There are few things which I am not sure why we need to have/do: - Do we really need to run the detection every time we start playing the strip? Intuitively, you look into the file when you drop it, create Video, Audio, or Image strip, but the specific strip type tries to interpret the file according to the strip type. This means, that, say, Video strip will always consider the file being a video, and use FFmpeg. - Why do we need `ANIM_SEQUENCE` type of `ImBufAnim`? Do we ever use `ImBufAnim` to access image sequences? I thought we use ImBuf for it, not the anim API.

Aras Pranckevicius commented

2024-02-20 14:48:13 +01:00

Good catch wrt .ogg. Whoops!

So, I guess:

for OGG (i.e. IMB_isanim) usage, keep existing code with full-fledged detection.
for VSE playback (inside anim_getnew), there's no need to do full-blown detection since that is immediately followed by "try to actually initialize the decoder".

I have no idea what ANIM_SEQUENCE is, the code was already there. I can try to figure that out.

Good catch wrt `.ogg`. Whoops! So, I guess: - for OGG (i.e. `IMB_isanim`) usage, keep existing code with full-fledged detection. - for VSE playback (inside `anim_getnew`), there's no need to do full-blown detection since that is immediately followed by "try to actually initialize the decoder". I have no idea what `ANIM_SEQUENCE` is, the code was already there. I can try to figure that out.

Sergey Sharybin commented

2024-02-20 15:05:42 +01:00

I think we can keep IMB_isanim() as-is. Or, rather, for achieving the goal of lower latency on strip cuts I do not think optimization IMB_isanim() is the most ultimate way, but it might still worth optimizing it for other usecases.

For the latency on cuts I think the most interesting would be to "just" get rid of ImBufAnim::curtype. This will imply that the ImbAnim is only used for video files, which I think is a fair point. I do not know why we'd want an extra abstraction level for image sequences. The Image data-block does not rely on it, and quite sure the Image strip does not either. It could be some historical rudiment.

Doing so will remove even more code around ImbAnimType, imb_get_anim_type (make it purely boolean IMB_isanim), and will also avoid an extra avformat_open_input call upon animation start.

I think we can keep `IMB_isanim()` as-is. Or, rather, for achieving the goal of lower latency on strip cuts I do not think optimization `IMB_isanim()` is the most ultimate way, but it might still worth optimizing it for other usecases. For the latency on cuts I think the most interesting would be to "just" get rid of `ImBufAnim::curtype`. This will imply that the `ImbAnim` is only used for video files, which I think is a fair point. I do not know why we'd want an extra abstraction level for image sequences. The `Image` data-block does not rely on it, and quite sure the Image strip does not either. It could be some historical rudiment. Doing so will remove even more code around `ImbAnimType`, `imb_get_anim_type` (make it purely boolean `IMB_isanim`), and will also avoid an extra `avformat_open_input` call upon animation start.

👍 1

Aras Pranckevicius added 2 commits 2024-02-20 19:22:06 +01:00

ba28e95155 Revert "VSE: Speedup isffmpeg by doing a simpler check"

This reverts commit db9aeff87b.

buildbot/vexp-code-patch-lint Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

buildbot/vexp-code-patch-coordinator Build done. Details

f090fe1dad Video: simplify ImBufAnim

- It looks like ImBufAnim is only ever used to play back "video files",
  so remove all the other modes of operation from it ("image sequence").
- Which makes ImBufAnim::curtype be not needed, it only needs to store
  state of whether it's initialized already.
- Which means there's no need to call imb_get_anim_type (which does very
  costly isffmpeg) when starting to play ImBufAnim.
- Remove some other variables from ImBufAnim that were just flat out
  unused.

In Gold previs playback between 1:41-1:55, on Windows/Ryzen5950X:
- Slowest 3 frames went from 276, 195, 168ms -> 222, 174, 147ms
  (saves 20+ ms per frame). All of these frames are camera cuts where
  multiple new video clips start playing.
- In the whole playback, total amount of time taken by
  `imb_get_anim_type`: 234ms -> zero! Since that is no longer used.
- There are still stalls when starting to play a movie clip, and that is
  actually initializing ffmpeg things. But now at least right before
  "initialize ffmpeg" there's no additional redundant work.

Sergey Sharybin requested changes 2024-02-21 09:49:49 +01:00

Sergey Sharybin left a comment

Unfortunately, it turns out the FFmpegIndexBuilderContext and IndexBuildContext are potentially of sync, and this change definitely makes them to go out of sync.

This is because of C-style sub-classing and casts to base class: the head of FFmpegIndexBuilderContext is expected to match IndexBuildContext, as there are reinterpret casts between them.

I think we need to make this to be real C++ sub-classing, and maybe sprincle some smart pointers in the index_ffmpeg_create_context. And eventually move all functions as virtual methods.

For now it will suffice to simply define empty IndexBuildContext, and make FFmpegIndexBuilderContext subclass of it (and replace MME_cnew with MEM_new, and MEM_freeN with MEM_delete for the context)

Unfortunately, it turns out the `FFmpegIndexBuilderContext` and `IndexBuildContext` are potentially of sync, and this change definitely makes them to go out of sync. This is because of C-style sub-classing and casts to base class: the head of `FFmpegIndexBuilderContext` is expected to match `IndexBuildContext`, as there are reinterpret casts between them. I think we need to make this to be real C++ sub-classing, and maybe sprincle some smart pointers in the `index_ffmpeg_create_context`. And eventually move all functions as virtual methods. For now it will suffice to simply define empty `IndexBuildContext`, and make `FFmpegIndexBuilderContext` subclass of it (and replace `MME_cnew` with `MEM_new`, and `MEM_freeN` with `MEM_delete` for the context)

Aras Pranckevicius added 2 commits 2024-02-21 10:17:09 +01:00

0baa42e282 Merge branch 'main' into speedup_isffmpeg

# Conflicts:
#	source/blender/imbuf/intern/util.cc

buildbot/vexp-code-patch-lint Build done. Details

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-coordinator Build done. Details

dbcb56dc3f Fix FFmpegIndexBuilderContext subclassing

Aras Pranckevicius commented

2024-02-21 11:59:01 +01:00

@blender-bot build

Aras Pranckevicius requested review from Sergey Sharybin 2024-02-21 12:11:54 +01:00

Aras Pranckevicius added 1 commit 2024-02-22 08:47:42 +01:00

08eff1c8ca IndexBuildContext::valid is not needed

Sergey Sharybin approved these changes 2024-02-22 09:08:39 +01:00

Aras Pranckevicius merged commit b261654a93 into main

2024-02-22 09:24:45 +01:00

Aras Pranckevicius referenced this issue from a commit

2024-02-22 09:24:46 +01:00

VSE: Reduce playback stalls when new video clips start playing

Aras Pranckevicius deleted branch speedup_isffmpeg

2024-02-22 09:24:48 +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: Reduce playback stalls when new video clips start playing #118503