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.

Sign up for GitHub

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

Jump to bottom

Wrong Bounds Check for pic_parameter_set_id #56

Closed
wrv opened this issue Jul 19, 2023 · 4 comments · Fixed by #64
Closed

Wrong Bounds Check for pic_parameter_set_id #56

wrv opened this issue Jul 19, 2023 · 4 comments · Fixed by #64

Comments

@wrv
Copy link

wrv commented Jul 19, 2023

The pic_parameter_set_id is a ParamSetId struct, which checks if the ID is within [0, 31]: https://github.com/dholroyd/h264-reader/blob/master/src/nal/pps.rs#L218

Valid bitstreams can have pic_parameter_set_ids in the range [0, 255].
@dholroyd
Copy link
Owner

This arises from mistakenly sharing a single ParamSetId type between seq_parameter_set_rbsp() and pic_parameter_set_rbsp() syntax. Will need to break this into two separate Id types.

astraw added a commit to strawlab/strand-braid that referenced this issue Oct 11, 2023
@astraw
frame-source: workaround h264-reader bug
71e937a 
workaround dholroyd/h264-reader#56
@astraw
Copy link
Contributor

astraw commented Oct 11, 2023

Is there a need for a video in which this gets triggered? I can provide one.

dholroyd added a commit that referenced this issue Feb 23, 2024
@dholroyd
New PicParamSetId to perform proper bounds check
d1c48c6 
Using a single ParamSetId had the problem that Ids for pic_parameter_set
and seq_parameter_set have different maximum allowed values.  This change
removes that original type and replaces it with seperate PicParamSetId and
SeqParamSetId types.

SeqParamSetId enforces a limit of 31 as before, but PicParamSetId enforces a
limit of 255.

Fixes #56
dholroyd added a commit that referenced this issue Feb 23, 2024
@dholroyd
New PicParamSetId to perform proper bounds check
279910e 
Using a single ParamSetId had the problem that Ids for pic_parameter_set
and seq_parameter_set have different maximum allowed values.  This change
removes that original type and replaces it with seperate PicParamSetId and
SeqParamSetId types.

SeqParamSetId enforces a limit of 31 as before, but PicParamSetId enforces a
limit of 255.

Fixes #56
@dholroyd dholroyd mentioned this issue Feb 23, 2024
Merged
dholroyd added a commit that referenced this issue Feb 23, 2024
@dholroyd
New PicParamSetId to perform proper bounds check
ebe2468 
Using a single ParamSetId had the problem that Ids for pic_parameter_set
and seq_parameter_set have different maximum allowed values.  This change
removes that original type and replaces it with seperate PicParamSetId and
SeqParamSetId types.

SeqParamSetId enforces a limit of 31 as before, but PicParamSetId enforces a
limit of 255.

Fixes #56
@dholroyd
Copy link
Owner

@astraw yes please - I would be happy to add a real PPS as a unit test

@astraw
Copy link
Contributor

astraw commented Feb 27, 2024 •
edited
Loading

Here is an MP4 file containing an h264 video track that triggers the issue for me. (I moved my .h264 file into an .mp4 file with ffmpeg -i movie20221006_115500.885150194-issue56.h264 -vcodec copy movie20221006_115500.885150194-issue56.mp4 because github didn't accept an .h264 upload. I verified the .h264 file extracted with ffmpeg -i movie20221006_115500.885150194-issue56.mp4 -vcodec copy extract.h264 still triggers the issue for me.)

dholroyd added a commit that referenced this issue Feb 27, 2024
@dholroyd
New PicParamSetId to perform proper bounds check
10fd58b 
Using a single ParamSetId had the problem that Ids for pic_parameter_set
and seq_parameter_set have different maximum allowed values.  This change
removes that original type and replaces it with seperate PicParamSetId and
SeqParamSetId types.

SeqParamSetId enforces a limit of 31 as before, but PicParamSetId enforces a
limit of 255.

Fixes #56
dholroyd added a commit that referenced this issue Feb 27, 2024
@dholroyd
New PicParamSetId to perform proper bounds check
6fa1be5 
Using a single ParamSetId had the problem that Ids for pic_parameter_set
and seq_parameter_set have different maximum allowed values.  This change
removes that original type and replaces it with seperate PicParamSetId and
SeqParamSetId types.

SeqParamSetId enforces a limit of 31 as before, but PicParamSetId enforces a
limit of 255.

Fixes #56
dholroyd added a commit that referenced this issue Feb 27, 2024
@dholroyd
New PicParamSetId to perform proper bounds check
1c353b1 
Using a single ParamSetId had the problem that Ids for pic_parameter_set
and seq_parameter_set have different maximum allowed values.  This change
removes that original type and replaces it with seperate PicParamSetId and
SeqParamSetId types.

SeqParamSetId enforces a limit of 31 as before, but PicParamSetId enforces a
limit of 255.

Fixes #56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

New PicParamSetId to perform proper bounds check dholroyd/h264-reader
3 participants
@astraw @dholroyd @wrv