Add save_video util and deprecate RecordVideo in favor of it

[younik]

This PR adds a deprecation warning to RecordVideo, in favor of a cleaner util function https://github.com/younik/gym/blob/record-video/gym/utils/save_video.py

See also #2905

Sample usage:

import gym
from gym.utils.save_video import save_video

env = gym.make("MyEnv", render_mode="rgb_array")

env.reset()
step_starting_index = 0
episode_index = 0
for step_index in range(199):
    action = env.action_space.sample()
    _, _, done, _ = env.step(action)

    if done:
        save_video(
             env.render(), 
             "videos", 
             fps=env.metadata["render_fps"], 
             step_starting_index=step_starting_index, 
             episode_index=episode_index
        )
        step_starting_index = step_index + 1
        episode_index += 1
        env.reset()

env.close()

[pseudo-rnd-thoughts]

Generally looks good, I added two comments for minor code changes

[vwxyzjn]

Thanks @younik for preparing the PR. What is the motivation for this change and why is using the save_video function cleaner? It looks to me save_video only adds more boilerplate code to the learning code: we need to maintain step_starting_idx and episode_idx and probably a scheduler logic to not record every episode.

Deprecating RecordVideo should perhaps serve a practical purpose.

[vwxyzjn]

I am very confused - why would episode_trigger still be here? It seems strange when users execute save_video the videos might not be saved…

[younik]

In the beginning, the expected type of frames was a List of episodes, where an episode is a List of frames.
Talking with @pseudo-rnd-thoughts, we agree it is simpler as it is now, with frames as a List of frames of the same episode.

For how it is right now, yes, we can drop episode_trigger and the user can just do it with an if statement.
Notice that it still can happen that save_video doesn't save anything depending on step_trigger

[vwxyzjn]

It looks like here only calls env.render() when the env is done. Does this mean when calling env = gym.make("FrozenLake-v1", render_mode="rgb_array") each frame is rendered and cached? If this is the case this could be an undesirable bottleneck — we might only keep the rendered frames of a few episodes.

[younik]

Yes, with the current Render API env.render() returns the list of frames of the whole episode when the mode is rgb_array; old-like behavior can be obtained with single_rgb_array.

When you call render() or reset(), the frame list is cleaned

[vwxyzjn]

Sorry, I am struggling to understand this. If I don’t want to render every frame, how can I achieve it with the current API? What is the old-like behavior with single_rgb_array?

[younik]

If you want to render a single frame representing the current state, you can achieve it with single_rgb_array, thus similar to before:

env = gym.make("MyEnv", render_mode="single_rgb_array")
env.reset()

for _ in range(n_steps):
    env.step(...)
    env.render()  # single frame

env.close()

Otherwise, with rgb_array:

env = gym.make("MyEnv", render_mode="rgb_array")
env.reset()

for _ in range(n_steps):
    _, _, done, _ = env.step(...)
   if done:
      break

env.render()  # List of all frames
env.close()

[younik]

Thanks @younik for preparing the PR. What is the motivation for this change and why is using the save_video function cleaner? It looks to me save_video only adds more boilerplate code to the learning code: we need to maintain step_starting_idx and episode_idx and probably a scheduler logic to not record every episode.

Deprecating RecordVideo should perhaps serve a practical purpose.

Hello, yes, this adds more boilerplate to user code, on the other side, save_video function is clearer than RecordVideo and fits better with the new behavior of rgb_array.

[vwxyzjn]

Could we do both? I think having the save_video as a low-level API would be helpful, but RecordVideo should be a no-brainer utility to most researchers.

[younik]

Could we do both? I think having the save_video as a low-level API would be helpful, but RecordVideo should be a no-brainer utility to most researchers.

Sure, I updated VideoRecorder to use moviepy on the other PR, thus it is easy to see the differences. For me, we can keep both, or stick with one

[pseudo-rnd-thoughts]

Personally, I think we should have both.
The wrapper for ease of use and a separate function for users who want more control

[vwxyzjn]

LGTM