-
Notifications
You must be signed in to change notification settings - Fork 11
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
decoder/stateless: let the backends decide which resources they need #89
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
new_picture is only used to allocate the resources needed for a new frame and doesn't need this argument.
The decoder code was checking explicitly for output buffers availability before decoding frames, but that requirement is not needed by all backends - for example V4L2 stateless has different requirements. Move this check to the backend by using the new_picture() method to try and allocate all required resources for decoding a frame.
Gnurou
changed the title
stateless/decoder: let the backends decide which resources they need
decoder/stateless: let the backends decide which resources they need
Jul 12, 2024
bgrzesik
reviewed
Jul 12, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left one comment with a "mayor nit" 😅 Otherwise LGTM 😄
The decoder code was checking explicitly for output buffers availability before decoding frames, but that requirement is not needed by all backends - for example V4L2 stateless has different requirements. Move this check to the backend by introducing a new_picture() method that is responsible for allocating the resources required for a picture and returning the expected error if it cannot secure them.
handle_frame cannot change the decoding state, so check for it before processing all frames.
…are decoding There is no reason to not pursue if we are not in a decoding state, so move that check to a more appropriate place.
This will allow us to allocate all the frames required prior to processing a superframe and make sure the whole superframe can be processed in one go.
…ethod This is a different procedure from decoding an regular frame, which justifies having both separate. This will also let us pass the picture to decode into to `handle_frame` without having to worry whether it will be written or not.
…rames If a frame has its show_existing_frame flag set, then it does not require an output resource.
The decoder code was checking explicitly for output buffers availability before decoding frames, but that requirement is not needed by all backends - for example V4L2 stateless has different requirements. Move this check to the backend by using the new_picture() method to try and allocate all required pictures of a superframe prior to decoding it.
This argument is not needed when creating a new resource.
…e::new_from_slice
This can be set directly from the slice header's information.
…es availability We don't need any resource to perform negotiation, so do that check first.
The only time we access this member is shortly after updating it with update_current_set_ids() from the slice's parameter. So let's just use the slice data instead of adding state that may become invalid.
This is some frame-local state that we would eventually like to get rid of, so don't use it when we can get the SPS ID through other means.
The decoder code was checking explicitly for output buffers availability before decoding frames, but that requirement is not needed by all backends - for example V4L2 stateless has different requirements. Move this check to the backend by using the new_picture() method to try and allocate a picture before processing it, and return the appropriate error if none is available.
decode() was trying to process a whole temporal unit, which complicates the code quite a bit: the decoder must notably check for available resources for all the frames in the OBU, which requires to parse the input several times. Our contract is that the caller must check how much input has been consumed and re-submit the remainder if it has not been entirely processed. Leverage this and only process one OBU per call to decode(), which simplifies the resource availability check and will also allow us to move it into the backend.
…tion We want to create pictures early so the backend can check whether all required resources are available before processing. This requires splitting new_picture into a method that allocates the picture and its resources, and another one that initializes the picture parameters, so they can be called from two different sites.
The decoder code was checking explicitly for output buffers availability before decoding frames, but that requirement is not needed by all backends - for example V4L2 stateless has different requirements. Move this check to the backend by using the new_picture() method to try and allocate a picture before processing it, and return the appropriate error if none is available.
bgrzesik
approved these changes
Jul 15, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Failures in new_picture are usually due to temporary unavailability of required resources and are recoverable. Introduce a new error type to better handle this, and try to be more precise about the type of resource missing so the client can handle each case properly.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This series fixes a design shortcoming in the decoder, namely that it was checking for output buffers availability before deciding to initiate a picture with the backend.
This was working well for the VAAPI backend, which must secure its output surface to create a
VAPicture
, but is not appropriate for V4L2 where the input and output queues are completely independent.Each codec has a series of CLs culminating with
let the backend decide which resources it needs
, which moves the burden of checking output surfaces availability from the decoder to the VAAPI backend. That way the V4L2 backend is not constrained by output resources, and can decide itself which resources it needs to acquire to create a picture (in its case, an input buffer).The other CLs include changes that are necessary to reach this state, e.g. rework of the backend interface to separate picture creation from submission when needed, and reordering of the format negotiation flow.
Sorry for the number of CLs ; however they should be bite-sized and relatively easy to understand.