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

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

[Andreas Rheinhardt]

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)
+            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++) {
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

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

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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [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@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.


_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".