[FFmpeg-devel] [PATCH v8 01/15] avcodec/vaapi_encode: introduce a base layer for vaapi encode

[Wu, Tong1]

>-----Original Message-----
>From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Mark
>Thompson
>Sent: Wednesday, May 15, 2024 3:56 AM
>To: ffmpeg-devel at ffmpeg.org
>Subject: Re: [FFmpeg-devel] [PATCH v8 01/15] avcodec/vaapi_encode:
>introduce a base layer for vaapi encode
>
>On 18/04/2024 09:58, tong1.wu-at-intel.com at ffmpeg.org wrote:
>> From: Tong Wu <tong1.wu at intel.com>
>>
>> Since VAAPI and future D3D12VA implementation may share some common
>parameters,
>> a base layer encode context is introduced as vaapi context's base.
>>
>> Signed-off-by: Tong Wu <tong1.wu at intel.com>
>> ---
>>  libavcodec/hw_base_encode.h | 52
>+++++++++++++++++++++++++++++++++++++
>>  libavcodec/vaapi_encode.h   | 36 ++++---------------------
>>  2 files changed, 57 insertions(+), 31 deletions(-)
>>  create mode 100644 libavcodec/hw_base_encode.h
>>
>> diff --git a/libavcodec/hw_base_encode.h b/libavcodec/hw_base_encode.h
>> new file mode 100644
>> index 0000000000..3d1974bba3
>> --- /dev/null
>> +++ b/libavcodec/hw_base_encode.h
>> @@ -0,0 +1,52 @@
>> +/*
>> + * This file is part of FFmpeg.
>> + *
>> + * FFmpeg is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * FFmpeg is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with FFmpeg; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
>USA
>> + */
>> +
>> +#ifndef AVCODEC_HW_BASE_ENCODE_H
>> +#define AVCODEC_HW_BASE_ENCODE_H
>> +
>> +#define MAX_DPB_SIZE 16
>> +#define MAX_PICTURE_REFERENCES 2
>> +#define MAX_REORDER_DELAY 16
>> +#define MAX_ASYNC_DEPTH 64
>> +#define MAX_REFERENCE_LIST_NUM 2
>
>Is there a reason to change these from enum to defines?  I'm not seeing
>anywhere they should be visible to the preprocessor, and this means they are
>normally invisible to a debugger.

It was a enum structure before V7 until Lynne suggested "Define these, don't enum unnecessarily".

>
>> +
>> +enum {
>> +    PICTURE_TYPE_IDR = 0,
>> +    PICTURE_TYPE_I   = 1,
>> +    PICTURE_TYPE_P   = 2,
>> +    PICTURE_TYPE_B   = 3,
>> +};
>> +
>> +enum {
>> +    // Codec supports controlling the subdivision of pictures into slices.
>> +    FLAG_SLICE_CONTROL         = 1 << 0,
>> +    // Codec only supports constant quality (no rate control).
>> +    FLAG_CONSTANT_QUALITY_ONLY = 1 << 1,
>> +    // Codec is intra-only.
>> +    FLAG_INTRA_ONLY            = 1 << 2,
>> +    // Codec supports B-pictures.
>> +    FLAG_B_PICTURES            = 1 << 3,
>> +    // Codec supports referencing B-pictures.
>> +    FLAG_B_PICTURE_REFERENCES  = 1 << 4,
>> +    // Codec supports non-IDR key pictures (that is, key pictures do
>> +    // not necessarily empty the DPB).
>> +    FLAG_NON_IDR_KEY_PICTURES  = 1 << 5,
>> +};
>> +
>> +#endif /* AVCODEC_HW_BASE_ENCODE_H */
>> ...
>
>Would it make more sense to put the HWBaseEncodeContext in this patch as
>well?  (With just the AVClass member.)
>

Yes and will do it.

Thanks,
Tong