[FFmpeg-devel] [PATCH] avcodec/hevc_ps: Improve PPS SCC extension bit depth check

[Andreas Rheinhardt]

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 at 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.