| Message ID | 20170509152955.9092-1-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, May 09, 2017 at 12:29:55PM -0300, James Almer wrote: > The code was skipping the entire reported SEI message size regardless of > the amount of bits read. > While in theory safe for NALU where the picture timing SEI message is alone > or at the end as we're using the checked bitstream reader, it isn't in any > other situation, where every SEI message in the NALU after the picture > timing one would potentially fail to parse. > > Change the function name to one more in line with the rest of file, and > remove the bogus "Skipped SEI" debug message while at it. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > No test case, all the files i checked plus those in the FATE suite seem to > have one SEI message per NALU, or the Picture Timing SEI as the last one. > > libavcodec/hevc_sei.c | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) I think the "skip to match the size" should be done in a single place for all SEI units and parsing should probably continue even if a SEI unit has a failure of some kind. the patch is fine too ... [...]
On 5/10/2017 8:06 AM, Michael Niedermayer wrote: > On Tue, May 09, 2017 at 12:29:55PM -0300, James Almer wrote: >> The code was skipping the entire reported SEI message size regardless of >> the amount of bits read. >> While in theory safe for NALU where the picture timing SEI message is alone >> or at the end as we're using the checked bitstream reader, it isn't in any >> other situation, where every SEI message in the NALU after the picture >> timing one would potentially fail to parse. >> >> Change the function name to one more in line with the rest of file, and >> remove the bogus "Skipped SEI" debug message while at it. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> No test case, all the files i checked plus those in the FATE suite seem to >> have one SEI message per NALU, or the Picture Timing SEI as the last one. >> >> libavcodec/hevc_sei.c | 15 +++++++-------- >> 1 file changed, 7 insertions(+), 8 deletions(-) > > I think the "skip to match the size" should be done in a single place > for all SEI units and parsing should probably continue even if a SEI > unit has a failure of some kind. Might look into this later. > > the patch is fine too ... Pushed, thanks.
diff --git a/libavcodec/hevc_sei.c b/libavcodec/hevc_sei.c index c5054bfaab..0ecf00c14c 100644 --- a/libavcodec/hevc_sei.c +++ b/libavcodec/hevc_sei.c @@ -124,8 +124,8 @@ static int decode_nal_sei_display_orientation(HEVCSEIDisplayOrientation *s, GetB return 0; } -static int decode_pic_timing(HEVCSEIContext *s, GetBitContext *gb, const HEVCParamSets *ps, - void *logctx) +static int decode_nal_sei_pic_timing(HEVCSEIContext *s, GetBitContext *gb, const HEVCParamSets *ps, + void *logctx, int size) { HEVCSEIPictureTiming *h = &s->picture_timing; HEVCSPS *sps; @@ -146,7 +146,11 @@ static int decode_pic_timing(HEVCSEIContext *s, GetBitContext *gb, const HEVCPar } get_bits(gb, 2); // source_scan_type get_bits(gb, 1); // duplicate_flag + skip_bits1(gb); + size--; } + skip_bits_long(gb, 8 * size); + return 1; } @@ -272,12 +276,7 @@ static int decode_nal_sei_prefix(GetBitContext *gb, HEVCSEIContext *s, const HEV case HEVC_SEI_TYPE_DISPLAY_ORIENTATION: return decode_nal_sei_display_orientation(&s->display_orientation, gb); case HEVC_SEI_TYPE_PICTURE_TIMING: - { - int ret = decode_pic_timing(s, gb, ps, logctx); - av_log(logctx, AV_LOG_DEBUG, "Skipped PREFIX SEI %d\n", type); - skip_bits(gb, 8 * size); - return ret; - } + return decode_nal_sei_pic_timing(s, gb, ps, logctx, size); case HEVC_SEI_TYPE_MASTERING_DISPLAY_INFO: return decode_nal_sei_mastering_display_info(&s->mastering_display, gb); case HEVC_SEI_TYPE_CONTENT_LIGHT_LEVEL_INFO:
The code was skipping the entire reported SEI message size regardless of the amount of bits read. While in theory safe for NALU where the picture timing SEI message is alone or at the end as we're using the checked bitstream reader, it isn't in any other situation, where every SEI message in the NALU after the picture timing one would potentially fail to parse. Change the function name to one more in line with the rest of file, and remove the bogus "Skipped SEI" debug message while at it. Signed-off-by: James Almer <jamrial@gmail.com> --- No test case, all the files i checked plus those in the FATE suite seem to have one SEI message per NALU, or the Picture Timing SEI as the last one. libavcodec/hevc_sei.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)