| Message ID | GV1P250MB0737BA1543E62F023066928E8F30A@GV1P250MB0737.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | Accepted |
| Commit | f6dc85ae1610b6aae40fc3c8eb61791d71ea00ab |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/hevc_ps: Improve PPS SCC extension bit depth check | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On 7/9/2023 9:19 PM, Andreas Rheinhardt wrote: > From the spec: "It is a requirement of bitstream conformance that > the value of luma_bit_depth_entry_minus8 shall be equal to > the value of bit_depth_luma_minus8"; similarly for chroma. > > Should fix Coverity ticket #1529226. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavcodec/hevc_ps.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c > index 4c4c1e2c17..1db2d3a242 100644 > --- a/libavcodec/hevc_ps.c > +++ b/libavcodec/hevc_ps.c > @@ -1581,11 +1581,13 @@ static int pps_scc_extension(GetBitContext *gb, AVCodecContext *avctx, > } > pps->monochrome_palette_flag = get_bits1(gb); > pps->luma_bit_depth_entry = get_ue_golomb_31(gb) + 8; > - if (!pps->monochrome_palette_flag) > - pps->chroma_bit_depth_entry = get_ue_golomb_31(gb) + 8; > - > - if (pps->chroma_bit_depth_entry > 16 || pps->chroma_bit_depth_entry > 16) Is Coverity complaining about this duplicate check? If so, you should mention that fixing the Coverity issue is a side effect of the compliance check you're applying, rather than the actual objective of the change. You could also replace one chroma_bit_depth_entry check with luma_bit_depth_entry in one commit, fixing the Coverity issue, then this compliance change in another. > + if (pps->luma_bit_depth_entry != sps->bit_depth) > return AVERROR_INVALIDDATA; > + if (!pps->monochrome_palette_flag) { > + pps->chroma_bit_depth_entry = get_ue_golomb_31(gb) + 8; > + if (pps->chroma_bit_depth_entry != sps->bit_depth_chroma) > + return AVERROR_INVALIDDATA; > + } > > num_comps = pps->monochrome_palette_flag ? 1 : 3; > for (int comp = 0; comp < num_comps; comp++) { LGTM either way.
James Almer: > On 7/9/2023 9:19 PM, Andreas Rheinhardt wrote: >> From the spec: "It is a requirement of bitstream conformance that >> the value of luma_bit_depth_entry_minus8 shall be equal to >> the value of bit_depth_luma_minus8"; similarly for chroma. >> >> Should fix Coverity ticket #1529226. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >> --- >> libavcodec/hevc_ps.c | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c >> index 4c4c1e2c17..1db2d3a242 100644 >> --- a/libavcodec/hevc_ps.c >> +++ b/libavcodec/hevc_ps.c >> @@ -1581,11 +1581,13 @@ static int pps_scc_extension(GetBitContext >> *gb, AVCodecContext *avctx, >> } >> pps->monochrome_palette_flag = get_bits1(gb); >> pps->luma_bit_depth_entry = get_ue_golomb_31(gb) + 8; >> - if (!pps->monochrome_palette_flag) >> - pps->chroma_bit_depth_entry = get_ue_golomb_31(gb) + 8; >> - >> - if (pps->chroma_bit_depth_entry > 16 || >> pps->chroma_bit_depth_entry > 16) > > Is Coverity complaining about this duplicate check? If so, you should > mention that fixing the Coverity issue is a side effect of the > compliance check you're applying, rather than the actual objective of > the change. It's indeed about this duplicate check. It's how I found this issue. > > You could also replace one chroma_bit_depth_entry check with > luma_bit_depth_entry in one commit, fixing the Coverity issue, then this > compliance change in another. > I don't see why separating this in two commits would be beneficial (except for my commit count, but I don't intentionally maximise that). Patches are typically split into smaller parts to ease reviewing, but this change is trivial either way (but a tiny bit less trivial when split). >> + if (pps->luma_bit_depth_entry != sps->bit_depth) >> return AVERROR_INVALIDDATA; >> + if (!pps->monochrome_palette_flag) { >> + pps->chroma_bit_depth_entry = get_ue_golomb_31(gb) + 8; >> + if (pps->chroma_bit_depth_entry != >> sps->bit_depth_chroma) >> + return AVERROR_INVALIDDATA; >> + } >> num_comps = pps->monochrome_palette_flag ? 1 : 3; >> for (int comp = 0; comp < num_comps; comp++) { > > LGTM either way.
diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c index 4c4c1e2c17..1db2d3a242 100644 --- a/libavcodec/hevc_ps.c +++ b/libavcodec/hevc_ps.c @@ -1581,11 +1581,13 @@ static int pps_scc_extension(GetBitContext *gb, AVCodecContext *avctx, } pps->monochrome_palette_flag = get_bits1(gb); pps->luma_bit_depth_entry = get_ue_golomb_31(gb) + 8; - if (!pps->monochrome_palette_flag) - pps->chroma_bit_depth_entry = get_ue_golomb_31(gb) + 8; - - if (pps->chroma_bit_depth_entry > 16 || pps->chroma_bit_depth_entry > 16) + if (pps->luma_bit_depth_entry != sps->bit_depth) return AVERROR_INVALIDDATA; + if (!pps->monochrome_palette_flag) { + pps->chroma_bit_depth_entry = get_ue_golomb_31(gb) + 8; + if (pps->chroma_bit_depth_entry != sps->bit_depth_chroma) + return AVERROR_INVALIDDATA; + } num_comps = pps->monochrome_palette_flag ? 1 : 3; for (int comp = 0; comp < num_comps; comp++) {
From the spec: "It is a requirement of bitstream conformance that the value of luma_bit_depth_entry_minus8 shall be equal to the value of bit_depth_luma_minus8"; similarly for chroma. Should fix Coverity ticket #1529226. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavcodec/hevc_ps.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)