search

chromeos / cros-codecs

BSD 3-Clause "New" or "Revised" License
30 stars 11 forks source link

Minor fixes #24

Closed dwlsalmeida closed 1 year ago

Gnurou commented 1 year ago

The matroska crate update makes the builder fail because Rust 1.70 is not deployed on Github's bots yet, but that should be very temporary. Merged everything except standardize flush() for now because I am not sure why we should wait for a keyframe after every flush - while I agree we typically will only do that at the end of a stream, is there a reason to enforce that?

dwlsalmeida commented 1 year ago

@Gnurou

Merged everything except standardize flush() for now because I am not sure why we should wait for a keyframe after every flush - while I agree we typically will only do that at the end of a stream, is there a reason to enforce that?

This is based on the GStreamer design.

https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/gst-plugins-bad/gst-libs/gst/codecs/gsth264decoder.c#L487

Notice how the DPB is drained and then cleared. You can't resume decoding after that, you must wait for a sync point. The only reason why we do not crash is because we happen to call that only at the end of decoding in ccdec. Client code can theoretically call this whenever they want though.

Note the VP9 version, which calls gst_vp9_decoder_reset, which is https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/gst-plugins-bad/gst-libs/gst/codecs/gstvp9decoder.c#L300

^ Again, must wait for a sync point

Same for VP8 -> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/gst-plugins-bad/gst-libs/gst/codecs/gstvp8decoder.c#L331

Gnurou commented 1 year ago

I see, thanks for clarifying. Indeed if we clear the DPB there is no way for us to resume decoding after that.

I am wondering how this works with something like a e.g. video editor that would send a few frames to decode, call flush() to get sure it gets the work it sent fast, and then sent some more P frames to decode what follows. Maybe I am misunderstand how such tools work and maybe they simply decode whole recoverable sets and keep the frames around to display as needed.

In any case we definitely don't want to crash after a flush, so merging this CL!