[FFmpeg-devel] [PATCH] avcodec/hevcdec: add support for sps_infer_scaling_list_flag == 1

[James Almer]

On 9/23/2024 2:07 PM, Anton Khirnov wrote:
> Quoting James Almer (2024-09-23 18:35:35)
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>> Untested, as i can't find a sample that triggers this code.
>>
>>   libavcodec/hevc/cabac.c |  3 ++-
>>   libavcodec/hevc/ps.c    | 18 +++++++++++++-----
>>   libavcodec/hevc/ps.h    |  1 +
>>   3 files changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/libavcodec/hevc/cabac.c b/libavcodec/hevc/cabac.c
>> index 892dd1c215..ac2b52986e 100644
>> --- a/libavcodec/hevc/cabac.c
>> +++ b/libavcodec/hevc/cabac.c
>> @@ -1084,8 +1084,9 @@ void ff_hevc_hls_residual_coding(HEVCLocalContext *lc, const HEVCPPS *pps,
>>           dc_scale = 16;
>>   
>>           if (sps->scaling_list_enabled && !(transform_skip_flag && log2_trafo_size > 2)) {
>> +            const HEVCLayerContext *const l = &s->layers[sps->sps_scaling_list_ref_layer_id];
>>               const ScalingList *sl = pps->scaling_list_data_present_flag ?
>> -            &pps->scaling_list : &sps->scaling_list;
>> +            &pps->scaling_list : &l->sps->scaling_list;
> 
> You need to check that the layer is active and actually has an SPS.
> 
>>               int matrix_id = lc->cu.pred_mode != MODE_INTRA;
>>   
>>               matrix_id = 3 * matrix_id + c_idx;
>> diff --git a/libavcodec/hevc/ps.c b/libavcodec/hevc/ps.c
>> index f18b88489b..64b3089967 100644
>> --- a/libavcodec/hevc/ps.c
>> +++ b/libavcodec/hevc/ps.c
>> @@ -1373,14 +1373,22 @@ int ff_hevc_parse_sps(HEVCSPS *sps, GetBitContext *gb, unsigned int *sps_id,
>>   
>>       sps->scaling_list_enabled = get_bits1(gb);
>>       if (sps->scaling_list_enabled) {
>> +        int sps_infer_scaling_list_flag = 0;
>> +
>>           set_default_scaling_list_data(&sps->scaling_list);
>>   
>> -        if (multi_layer_ext && get_bits1(gb)) { // sps_infer_scaling_list_flag
>> -            av_log(avctx, AV_LOG_ERROR, "sps_infer_scaling_list_flag=1 not supported\n");
>> -            return AVERROR_PATCHWELCOME;
>> -        }
>> +        if (multi_layer_ext)
>> +            sps_infer_scaling_list_flag = get_bits1(gb);
>>   
>> -        if (get_bits1(gb)) {
>> +        sps->sps_scaling_list_ref_layer_id = 0;
>> +        if (sps_infer_scaling_list_flag) {
>> +            sps->sps_scaling_list_ref_layer_id = get_bits(gb, 6);
>> +            if (sps->sps_scaling_list_ref_layer_id >= HEVC_VPS_MAX_LAYERS) {
> 
> This value is nuh_layer_id of the reference layer, but you're using it
> as an index into the VPS layer list. Mapping nuh_layer_id to VPS layer
> indices is done via vps->layer_idx.
> 
>> +                av_log(avctx, AV_LOG_ERROR, "Invalid value %d for sps_scaling_list_ref_layer_id",
>> +                       sps->sps_scaling_list_ref_layer_id);
>> +                return AVERROR_INVALIDDATA;
>> +            }
>> +        } else if (get_bits1(gb)) {
>>               ret = scaling_list_data(gb, avctx, &sps->scaling_list, sps);
>>               if (ret < 0)
>>                   return ret;
>> diff --git a/libavcodec/hevc/ps.h b/libavcodec/hevc/ps.h
>> index 6f5b1f8755..aa4326554c 100644
>> --- a/libavcodec/hevc/ps.h
>> +++ b/libavcodec/hevc/ps.h
>> @@ -277,6 +277,7 @@ typedef struct HEVCSPS {
>>       VUI vui;
>>       PTL ptl;
>>   
>> +    int sps_scaling_list_ref_layer_id;
> 
> Leading sps_ is redundant. Also might be better to store the index
> rather than layer id.
> 
> Overall I'm not sure it's worth it to support something for which no
> samples are known to exist.

Ok. We should request a sample then, and not just return PATCHWELCOME.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 495 bytes
Desc: OpenPGP digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20240923/e28d2691/attachment.sig>