encoder: Add VP9 support and refactor common encoder code

[bgrzesik]

This PR adds VP9 encoding support and refactors the common encoder code to remove code duplication between codecs.

[Gnurou]

Thanks, this is looking very good overall. A few inline comments in the code, and it would really make my life easier if you could split the big refactoring CL into its logical parts.

But overall the end result looks really good, so I'm looking forward to merging this!

[bgrzesik]

Thank you soo much for the review 😄

I will continue working on the remaining comments tomorrow. Commits before a8ce22b most likely won't change.

[bgrzesik]

Hopefully all comments were answered 😅 Thanks again for the review!

[Gnurou]

I have started merging up to encoder: Export codec specific types using trait (so please rebase), for which I have a comment. Once this is addressed/clarified, I think the whole series can go in! 👍

[Gnurou]

Technically (and unless I am missing the purpose of this) you don't need a generic parameter to this struct. This allows you to remove _phantom and keep this as an empty type.

After doing so, there is also no need to have a H264Specific type since H264 can just implement StatelessCodecSpecific and refer to itself as its own Specific type.

I.e. this works just fine (patch is on top of the whole series):

diff --git a/src/encoder/stateless/h264.rs b/src/encoder/stateless/h264.rs
index bc790fb69..93d56ec8c 100644
--- a/src/encoder/stateless/h264.rs
+++ b/src/encoder/stateless/h264.rs
@@ -158,14 +158,7 @@ where

 pub struct H264;

-pub struct H264Specific<Backend>
-where
-    Backend: StatelessVideoEncoderBackend<H264>,
-{
-    _phantom: std::marker::PhantomData<Backend>,
-}
-
-impl<Backend> StatelessCodecSpecific<H264, Backend> for H264Specific<Backend>
+impl<Backend> StatelessCodecSpecific<H264, Backend> for H264
 where
     Backend: StatelessVideoEncoderBackend<H264>,
 {
@@ -179,7 +172,7 @@ where
 }

 impl StatelessCodec for H264 {
-    type Specific<Backend> = H264Specific<Backend>
+    type Specific<Backend> = Self
          where Backend : StatelessVideoEncoderBackend<Self>;
 }

diff --git a/src/encoder/stateless/vp9.rs b/src/encoder/stateless/vp9.rs
index c17bd35a2..a261ffddb 100644
--- a/src/encoder/stateless/vp9.rs
+++ b/src/encoder/stateless/vp9.rs
@@ -83,14 +83,7 @@ pub struct BackendRequest<P, R> {

 pub struct VP9;

-pub struct VP9Specific<Backend>
-where
-    Backend: StatelessVideoEncoderBackend<VP9>,
-{
-    _phantom: std::marker::PhantomData<Backend>,
-}
-
-impl<Backend> StatelessCodecSpecific<VP9, Backend> for VP9Specific<Backend>
+impl<Backend> StatelessCodecSpecific<VP9, Backend> for VP9
 where
     Backend: StatelessVideoEncoderBackend<VP9>,
 {
@@ -104,7 +97,7 @@ where
 }

 impl StatelessCodec for VP9 {
-    type Specific<Backend> = VP9Specific<Backend>
+    type Specific<Backend> = VP9
          where Backend : StatelessVideoEncoderBackend<Self>;
 }

Once you do that, StatelessCodec is just used as a way to get an associated type to itself - so you can simplify things further:

diff --git a/src/encoder/stateless.rs b/src/encoder/stateless.rs
index b2c4dacaa..9d9adcd65 100644
--- a/src/encoder/stateless.rs
+++ b/src/encoder/stateless.rs
@@ -208,10 +208,10 @@ pub trait StatelessEncoderBackendImport<Handle, Picture> {
 }

 /// Trait helping contain all codec specific and backend specific types
-pub trait StatelessCodecSpecific<Codec, Backend>
+pub trait StatelessCodecSpecific<Backend>
 where
-    Codec: StatelessCodec,
-    Backend: StatelessVideoEncoderBackend<Codec>,
+    Self: StatelessCodec,
+    Backend: StatelessVideoEncoderBackend<Self>,
 {
     /// Codec specific representation of frame reference wrapping a backend reference type
     /// containing a codec specific frame metadata
@@ -229,12 +229,7 @@ where
     type ReferencePromise: BackendPromise<Output = Self::Reference>;
 }

-pub trait StatelessCodec: Sized {
-    /// Codec and backend specific types container
-    type Specific<Backend>: StatelessCodecSpecific<Self, Backend>
-    where
-        Backend: StatelessVideoEncoderBackend<Self>;
-}
+pub trait StatelessCodec: Sized {}

 /// Stateless video encoder interface.
 pub trait StatelessVideoEncoder<Handle> {
@@ -293,23 +288,20 @@ where
 /// Helper aliases for codec and backend specific types
 type Picture<C, B> = <B as StatelessVideoEncoderBackend<C>>::Picture;

-type CodecSpecific<C, B> = <C as StatelessCodec>::Specific<B>;
-
-type Reference<C, B> = <CodecSpecific<C, B> as StatelessCodecSpecific<C, B>>::Reference;
+type Reference<C, B> = <C as StatelessCodecSpecific<B>>::Reference;

-type Request<C, B> = <CodecSpecific<C, B> as StatelessCodecSpecific<C, B>>::Request;
+type Request<C, B> = <C as StatelessCodecSpecific<B>>::Request;

-type CodedPromise<C, B> = <CodecSpecific<C, B> as StatelessCodecSpecific<C, B>>::CodedPromise;
+type CodedPromise<C, B> = <C as StatelessCodecSpecific<B>>::CodedPromise;

-type ReferencePromise<C, B> =
-    <CodecSpecific<C, B> as StatelessCodecSpecific<C, B>>::ReferencePromise;
+type ReferencePromise<C, B> = <C as StatelessCodecSpecific<B>>::ReferencePromise;

 type BoxPredictor<C, B> = Box<dyn Predictor<Picture<C, B>, Reference<C, B>, Request<C, B>>>;

 pub struct StatelessEncoder<Codec, Handle, Backend>
 where
     Backend: StatelessVideoEncoderBackend<Codec>,
-    Codec: StatelessCodec,
+    Codec: StatelessCodec + StatelessCodecSpecific<Backend>,
 {
     /// Pending frame output promise queue
     output_queue: OutputQueue<CodedPromise<Codec, Backend>>,
@@ -340,14 +332,14 @@ where
 pub trait StatelessEncoderExecute<Codec, Handle, Backend>
 where
     Backend: StatelessVideoEncoderBackend<Codec>,
-    Codec: StatelessCodec,
+    Codec: StatelessCodec + StatelessCodecSpecific<Backend>,
 {
     fn execute(&mut self, request: Request<Codec, Backend>) -> EncodeResult<()>;
 }

 impl<Codec, Handle, Backend> StatelessEncoder<Codec, Handle, Backend>
 where
-    Codec: StatelessCodec,
+    Codec: StatelessCodec + StatelessCodecSpecific<Backend>,
     Backend: StatelessVideoEncoderBackend<Codec>,
     Self: StatelessEncoderExecute<Codec, Handle, Backend>,
 {
@@ -392,7 +384,7 @@ where
 impl<Codec, Handle, Backend> StatelessVideoEncoder<Handle>
     for StatelessEncoder<Codec, Handle, Backend>
 where
-    Codec: StatelessCodec,
+    Codec: StatelessCodec + StatelessCodecSpecific<Backend>,
     Backend: StatelessVideoEncoderBackend<Codec>,
     Backend: StatelessEncoderBackendImport<Handle, Backend::Picture>,
     Self: StatelessEncoderExecute<Codec, Handle, Backend>,
diff --git a/src/encoder/stateless/h264.rs b/src/encoder/stateless/h264.rs
index 93d56ec8c..3c03a6978 100644
--- a/src/encoder/stateless/h264.rs
+++ b/src/encoder/stateless/h264.rs
@@ -158,9 +158,9 @@ where

 pub struct H264;

-impl<Backend> StatelessCodecSpecific<H264, Backend> for H264
+impl<Backend> StatelessCodecSpecific<Backend> for H264
 where
-    Backend: StatelessVideoEncoderBackend<H264>,
+    Backend: StatelessVideoEncoderBackend<Self>,
 {
     type Reference = DpbEntry<Backend::Reconstructed>;

@@ -171,10 +171,7 @@ where
     type ReferencePromise = ReferencePromise<Backend::ReconPromise>;
 }

-impl StatelessCodec for H264 {
-    type Specific<Backend> = Self
-         where Backend : StatelessVideoEncoderBackend<Self>;
-}
+impl StatelessCodec for H264 {}

 /// Trait for stateless encoder backend for H.264
 pub trait StatelessH264EncoderBackend: StatelessVideoEncoderBackend<H264> {
diff --git a/src/encoder/stateless/vp9.rs b/src/encoder/stateless/vp9.rs
index a261ffddb..4b7ed4905 100644
--- a/src/encoder/stateless/vp9.rs
+++ b/src/encoder/stateless/vp9.rs
@@ -83,9 +83,9 @@ pub struct BackendRequest<P, R> {

 pub struct VP9;

-impl<Backend> StatelessCodecSpecific<VP9, Backend> for VP9
+impl<Backend> StatelessCodecSpecific<Backend> for VP9
 where
-    Backend: StatelessVideoEncoderBackend<VP9>,
+    Backend: StatelessVideoEncoderBackend<Self>,
 {
     type Reference = Backend::Reconstructed;

@@ -96,10 +96,7 @@ where
     type ReferencePromise = Backend::ReconPromise;
 }

-impl StatelessCodec for VP9 {
-    type Specific<Backend> = VP9
-         where Backend : StatelessVideoEncoderBackend<Self>;
-}
+impl StatelessCodec for VP9 {}

 pub trait StatelessVP9EncoderBackend<H>: StatelessVideoEncoderBackend<VP9> {
     fn encode_frame(

I don't know how much this makes sense, so please take it as a suggestion. With that StatelessCodec is not really useful anymore, hinting that maybe we can simplify some more. I'll let you see what should be done as I am not completely sure of your intent for the future here.

[bgrzesik]

No, you are right here 😄

My intent for the potential future use would be to take advantage of already existing decoder's (or similar) structures for keeping the decoder state definitely consistent instead relying on predictor for this. Second benefit of that would be that the predictor would be a single function trait which would accept a queue of pending frames and decoder state in order to make the decision of the next actions. But first of all currently I don't consider it as a deal breaking benefit now. And I think it could bring downsides of overcomplication, potentially engaging parser for no reason and breaking possible parallelism for GoP structure (WIP).

Back to the review. You are right regarding simplification of those traits. I even went ahead and merged StatelessCodec and StatelessCodecSpecific into one. And despite those simplifications I still believe the above idea continues to be possible to implement.

Also now that I think about it, shouldn't we also merge crate::decoder::stateless::h264::H264 and crate::encoder::stateless::h264::H264 (and the same for VP9)?

[Gnurou]

Since this has the same constraints has StatelessEncoder, and StatelessEncoder depends on this trait, can't we just make execute a method of StatelessEncoder?

[bgrzesik]

But in such case I'd have to add constraints for Handle and Backend as well

[Gnurou]

I guess we could replace these codec-specific methods with a single one if we add a few more associated types (EncoderConfig and Predictor) to StatelessCodec, but let's do this as a follow-up.

[Gnurou]

I don't see a hard guarantee that the unwrap won't crash us - it would be nice to remove it at some point.

[bgrzesik]

Oh, I should add a // SAFETY: comment. It is in the next_request(). If references is empty, this function would not be called.

[Gnurou]

All merged! (not from the GUI since I had to cargo format a few commits). There are a few inline comments, we can address them as follow-ups. Thanks for the great work!

[bgrzesik]

Thank you!!

All merged! (not from the GUI since I had to cargo format a few commits).

I am surprised to see that, I remember running the cargo fmt before upload. Maybe we have a discrepancy in rustfmt.toml. Could you please add yours to the repo?

[bgrzesik]

Oh, I should add a // SAFETY: comment. It is in the next_request(). If references is empty, this function would not be called.

[bgrzesik]

But in such case I'd have to add constraints for Handle and Backend as well