RecordVideo wrapper revived

[johnMinelli]

Removed deprecation warning to RecordVideo and VideoRecorder classes and updated to be compatible with new environment render mode (#2671)

VideoRecorder is now compatible with environments setted with one of the following render_mode : "ansi", "single_rgb_array", "rgb_array".

[younik]

Overall it LGTM; thanks a lot for addressing this.

You should probably override also the render method because with rgb_array the frames are popped.

For example:

env = RecordVideo(gym.make("FrozenLake-v1", render_mode='rgb_array'), './video')
env.reset()
for _ in range(10):
    env.step(env.action_space.sample())

> len(env.render())

doesn't return the expected result.

One minor comment is that this is not backwards compatible, i.e. it doesn't work with render_mode='None'.

Overall, I am actually unfulfilled about how this Wrapper code is complicated when with MoviePy you can do it with two lines. Obviously, this is not related to your PR @johnMinelli
I think that the main reason is the step trigger (and eventually the backward compatibility).
@vwxyzjn is the step trigger important?

[vwxyzjn]

Hi @younik, yes! Step trigger is especially important in high-throughput envs like Isaac Gym or Brax, where using episode_trigger would be more difficult when you have 2048 episodes. We might also want to render the entire scene of 2048 robotics regardless of the episodes like below, where the step trigger would be helpful.

[younik]

Hi @younik, yes! Step trigger is especially important in high-throughput envs like Isaac Gym or Brax, where using episode_trigger would be more difficult when you have 2048 episodes. We might also want to render the entire scene of 2048 robotics regardless of the episodes like below, where the step trigger would be helpful.

Clear, thanks!

@johnMinelli do you think it can be refactored using MoviePy?
I think video_recorder.py could be deleted and you can save the video using MoviePy (in the currently close_video_recorder). The MoviePy dependency should not be global (see pygame dependency as an example).

[johnMinelli]

@johnMinelli do you think it can be refactored using MoviePy? I think video_recorder.py could be deleted and you can save the video using MoviePy (in the currently close_video_recorder). The MoviePy dependency should not be global (see pygame dependency as an example).

In video_recorder.py there are class VideoRecorder, ImageEncoder and TextEncoder. Adopting MoviePy the ImageEncoder wouldn't be necessary anymore but i think that the other two should remain: VideoRecorder for matedata handling, render_mode check and the initialization of the ImageSequenceClip from MoviePy, while TextEncoder in case of "ansi" render_mode.

[younik]

In video_recorder.py there are class VideoRecorder, ImageEncoder and TextEncoder. Adopting MoviePy the ImageEncoder wouldn't be necessary anymore but i think that the other two should remain: VideoRecorder for matedata handling, render_mode check and the initialization of the ImageSequenceClip from MoviePy, while TextEncoder in case of "ansi" render_mode.

Regarding metadata, there is hacky code to maintain backward compatibility with render_fps and there are already the deprecation notes; we will remove that.
TextEncoder can also be deprecated, I think; in this way, we can switch to a cleaner code and delegate to MoviePy.
render_mode check can be done during RecordVideo initialization.

Thus, in addition to my previous comments, let's add a deprecation warning for ansi mode. Probably VideoRecorder is not used alone, but since it is public we should also add a deprecation warning for the class (that is turned off if used by RecordVideo). Later, we will switch to MoviePy; I open an issue.
After that, it should be ok for me; thank you @johnMinelli

[johnMinelli]

I wrote the code for the MoviePy proposal, I left 3 notes in record_video.py to review about the wanted behaviour.
The test build is failing due to missing moviepy dependency (I guess).

...oh, I just realized that maybe it would have been better to make a new pr? I'll drop the last commits in the case

[younik]

I wrote the code for the MoviePy proposal, I left 3 notes in record_video.py to review about the wanted behaviour.

Great, thanks! However, we cannot delete TextEncoder and VideoRecorder right now, but we need a period with deprecation warnings.
Your code is still valid for the future integration, but for now, I suggest branching from 91896ea and fixing the following:

• Fix render behaviour (frames are popped when you call it inside 'RecordVideo`, but this should be transparent to the user)
• Make it backwards compatible (it should work also with render_mode=None)
• And deprecation warnings for ansi, render_mode=None, and VideoRecorder (only if initialized outside RecordVideo, you can just use a toggle variable).

You will probably need to open a new PR; feel free to close this, open the new one referencing this one and ping me.

[johnMinelli]

Ok thank you, I'll do as you suggested 👍

[younik]

I added a couple of minor comments, but then it is ready for me!

[younik]

LGTM

[pzhokhov]

@johnMinelli Is CI currently testing the revived VideoRecorder? If no, could you add tests please?

[younik]

@johnMinelli Is CI currently testing the revived VideoRecorder? If no, could you add tests please?

Previous tests are still there: https://github.com/openai/gym/blob/master/tests/wrappers/test_record_video.py