Handover of final codec fixes #91
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
The proper way to wait for the next event is the poll the decoder's event file descriptor instead of calling decode() over and over again until an event is delivered, like our simple_decode_loop() helper currently does. Provide a decoder helper that does that and make it available as the public interface. While probably not useful for real-life scenarios, it is handy to have. This also allows us to timeout instead of looping infinitely if the decoder is starved for output buffers, as currently happens with Fluster's RPS_E_qualcomm_5 H.265 test vector.
This looks like a copy/paste error.
This parameter can be computed from the header. Accessing it from there makes the pps parameter of Picture::new_from_slice() unneeded, which simplifies the code a bit. For some reason this also makes Fluster's RPS_C_ericsson_5 test vector pass.
We are already doing this in H.264 to avoid having to make potentially failing calls to get_sps().
This will save us a few fights with the borrow checker.
Use the same remove() -> entry..or_insert pattern as we did for H.264 in order to remove these unwrap().
This will allow the Sps to reference it directly.
This can be accessed directly from the picture data so there is no need to duplicate it.
This error can be triggered from invalid input. We don't want to crash in that case. Also move the check to a more favorable position that removes the need to fetch the Pps a second time.
This is the appropriate level at which to access the state, and it allows us to remove a call to get_pps() by passing the PPS as an argument.
We can do this by cloning the Sps' Rc. Doing so save us one potentially failing lookup.
A currently processed frame needs to be submitted before we start a new one.
…ery point We don't need to parse the whole frame since only the header information is needed.
Slices let us manage the stream data without the need to always refer to start_offset, which is safer.
The OBU is not parsed at this stage, we are just deciding what to do with it. Rename accordingly to avoid confusion.
This separates the parsing of OBUs from their processing, which removes a distraction from the decoder code, and allows us to make a few parser methods private.
This method does not need to take a mutable reference.
Invalid streams can request us to read an arbitrarily high number of bits, but H.264 integers are never more than 31 bits. Stop early if this happens instead of potentially making things worse.
Change the order of operations so the negation operation cannot result in an overflow.
This removes another use of anyhow.
The DPB serves two purposes: as a way to reorder frames in display order, and as a way to mark frames that are still used as reference. Currently both purposes are served by the same `handle` member, but separating it into dedicated members brings benefits: the type of the decoded frame could be different from the one used for reference, and the decoded frame could be take()n away from an Option when the frame is bumped, making sure it cannot be emitted a second time. IOW, this should allow the removal of the Rcs to reference frames in the DPB, once a proper reference frame type is introduced.
One more step in the removal of anyhow.
Avoid redundant checks and make the code flow more naturally overall.
Flatten the conditional code blocks into a linear flow towards the success path.
This field is only used with interlaced streams, and in case the DPB is full when we try to add the first field of a frame to it. It has been confirmed with Daniel Almeida (who initially wrote this code) that this field was added as an extra safety to mimic something GStreamer does, but is not warranted by the specification itself. In practice, none of our unit tests or Fluster streams ever exercise a path where last_field is set, and its presence seriously complicates the code. Let's remove it.
We might get an out-of-bounds index with an invalid stream - check that the index is valid before addressing. This issue has been revealed by fuzzing.
rosetta-jpn
approved these changes
Oct 17, 2024
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.
Hi Hiro,
These are the changes that were pending on my local branches and weren't pushed yet. Mostly small and easy fixes as well as readability improvements, although H.264 has some more involved stuff to remove unneeded complexity. Some of the efforts are still not complete though, like the removal of
anyhow, where I just pushed what I had.I have ran the full Fluster suite with these changes and haven't seen any regression.