Fixes/improvements for the H.264 parser #82
Conversation
|
@bgrzesik I am mostly interested in your feedback for the first CL since it touches the encoder, but please feel free to look at the rest as this is the global direction I would like to move all the parsers towards (i.e. limit the range of read data to prevent runtime panics even with random or forged input). For the encoder side, I wonder whether we should not make some of the builder methods fallible if their input would result in a potentially panicking SPS. Actually I think we should do this, but would like your thoughts before proceeding. |
There was a problem hiding this comment.
Hi! I left a few comments.
For the encoder side, I wonder whether we should not make some of the builder methods fallible if their input would result in a potentially panicking SPS. Actually I think we should do this, but would like your thoughts before proceeding.
Fully agree and in this case it is fully justified. At the same time if we want to expose synthesizers like parsers I wonder how we can do error checking so we won't prevent producing an invalid bitstream in case someone would actually like to do it purposely.
Calling this method after `resolution` could potentially lead to an invalid state. The latter allows us to do all that `resolution_in_mbs` does, so consolidate around that one.
These members are not directly part of the parsed stream, but rather calculated from it. Replace them by methods to eliminate the risk of inconsistency.
The H.264 specification mentions that the maximum width of a stream is 8192 pixels (level 6.2). This means that pic_width_in_mbs_minus1 cannot exceed (8192 / 16) - 1, so change its type to u16 and reject input values that won't fit. This has the benefit of bounding the possible values for pic_width_in_mbs_minus1 in such a way that overflows in future operations become impossible.
The H.264 specification mentions that the maximum height of a stream is 4320 pixels (level 6.2). This means that pic_height_in_map_units_minus1 cannot exceed 270, so change its type to u16 and reject input values that won't fit. This has the benefit of bounding the possible values for pic_height_in_map_units_minus1 in such a way that overflows in future operations become impossible.
This is more accurate and removes the need for masking.
This is more accurate.
This can be detected by a checked_add, preventing doing the same operation twice in the code.
Make sure that read_ue performs as expected, particularly at its limits.
…ir own methods These are computed at at least two points in the code. Having them as dedicated methods also makes it easier to match the code with the spec.
Revealed by fuzzing.
These members need to be computed to validate the crop parameters, but there is no point in storing them since they are not used.
This is trivial to compute from the SPS, so do it instead of storing its value to avoid potential mismatches.
Its upper limit is 16, so this is a more appropriate type. It also removes the need to use try_from on it in the code.
Revealed by fuzzing.
A set of small changes that restrict the data types and valid ranges of some SPS parameters to make sure we don't get runtime panics due to overflows when processing invalid data.
Some of these problems have been revealed by fuzzing, some others are just drive-by improvements, notably those that move pre-computed values into methods to remove the possibility of inconsistencies and better align with the spec.