Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

decoders: Add stateful decoder API #62

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

InternetOfTofu
Copy link

Add an initial version of stateful decoder API.

Add an initial version of stateful decoder API.
@dwlsalmeida
Copy link
Collaborator

@InternetOfTofu can you provide at least one implementation so that we know this works? Also, IIRC, someone from @Gnurou 's team was already working on something similar

@Gnurou
Copy link
Collaborator

Gnurou commented Jan 8, 2024

That someone is actually @InternetOfTofu :)

Sorry for the delay, I'll try to review this this week!

Other(#[from] anyhow::Error),
}

#[derive(Copy, Clone, Debug)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since ccdec has a similar enum, we should probably have a single declaration in lib.rs since we already have DecodedFormat there.


#[derive(Copy, Clone, Debug)]

pub enum ColorGamut {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now we don't have any way to manage color properties, even for stateless decoders. Let's drop this for now as it is not needed for basic decoding.

Smpte170m,
}

#[derive(Copy, Clone, Debug)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Iec61966_2_1,
}

#[derive(Copy, Clone, Debug)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Smpte170m,
}

#[derive(Copy, Clone, Debug)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

buffer: Option<Box<dyn AsBytes>>,
}

pub enum StatefulDecoderEvent {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect you can reuse DecoderEvent (and the DecodedHandle trait) as-is for the stateful decoder. It will align the interfaces and make the code simpler somehow.

FrameReady(VideoFrame),
/// The format of the stream has changed and action is required. A VideoFrame
/// with no buffer is returned.
FormatChanged(VideoFrame),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to return a videoframe here?

Closed,
}

pub struct StatefulDecoder<C, B>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first use of the stateful decoder interface should be as a stateful wrapper to any stateless decoder, i.e. an adapter that turns a stateless decoder into a stateful one. That adapter maybe be implemented as a StatefulDecoderBackend, but for simplicity I'd recommend writing it as an implementation of StatefulDecoder first and see what parts we can share with other stateful decoder implementations.

So I'd suggest starting with the design of this wrapper first to guide the interface. Starting from any stateless decoder, the stateful wrapper should

  • Accept input buffers and queue them until they can be processed,
  • Try to submit work to the stateless decoder, hold if the work cannot be submitted to retry later, and process the events of the stateless decoder, forwarding them to the caller if needed (e.g. decoded frames),

... and I think that's mostly it. The simple_playback_loop should give a good example of how things should work, as its purpose is to provide some stateful-ish behavior to our stateless decoders. I will be replaced it by the stateful decoder once it is available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants