Handle H265 encoders that produce multi-slice IDR #844
Handle H265 encoders that produce multi-slice IDR #844chenosaurus wants to merge 6 commits intomainfrom
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
boks1971
left a comment
boks1971
left a comment
There was a problem hiding this comment.
A few comments @chenosaurus . LGTM, but I am not familiar with all the nuances of the bitstream. Getting some more 👀 on this would be great.
readersampleprovider.go
Outdated
| if *sampleIsAnnexB || len(dst) > 0 { | ||
| if !*sampleIsAnnexB && len(dst) > 0 { | ||
| dst = append([]byte{0, 0, 0, 1}, dst...) | ||
| *sampleIsAnnexB = true |
There was a problem hiding this comment.
should there be a return here? Not able to fully understand the sequence needed here, but letting this fall through seems like it adds another start code and also sets sampleIsAnnexB to true again. Seems redundant.
readersampleprovider.go
Outdated
| // aggregate vps,sps,pps into a single AP packet (chrome requires this) | ||
| // Aggregate VPS/SPS/PPS before the next access unit. | ||
| if nal.NalUnitType == 32 || nal.NalUnitType == 33 || nal.NalUnitType == 34 { | ||
| sampleIsAnnexB = true |
There was a problem hiding this comment.
sampleAnnexB is also getting set here. Would be good to consolidate AnnexB determination in one place if possible?
readersampleprovider.go
Outdated
| // Once we've started a picture, detect the next access-unit boundary. | ||
| if nal.NalUnitType < 32 { | ||
| // VCL: split when first_slice_segment_in_pic_flag starts a new picture. | ||
| if isFirstSlice, ok := h265FirstSliceInPic(nal.Data); ok && isFirstSlice { |
There was a problem hiding this comment.
can this be simplified a bit? looks like only the combination of firstInSlice: true && ok: false is the only one continuing. Can this be written as one check?
if isFirstSlice, ok := h265FirstSliceInPic(nal.Data); !ok || isFirstSlice
Did I get that logic right?
readersampleprovider.go
Outdated
| switch nal.NalUnitType { | ||
| case 40: // suffix SEI, ignore | ||
| continue | ||
| case 39, 32, 33, 34: |
There was a problem hiding this comment.
Is this case different from default below? If not, can this just be made a comment and made let the default be the handler?
There was a problem hiding this comment.
ur right, simplified
2 participants
Fix issue where a h265 encoder outputs multi-slice IDR, causing downstream decoders to fail.
first_slice_segment_in_pic_flagand collect the slices into a single AU before emitting the frame.