Re: [FFmpeg-devel] [PATCH v5 2/9] avcodec/vaapi_encode: introduce a base layer for vaapi encode

2024-02-22 Thread Wu, Tong1 
Hi Mark. Thanks for your careful review.

>On 18/02/2024 08:45, tong1.wu-at-intel@ffmpeg.org wrote:
>> From: Tong Wu 
>>
>> Since VAAPI and future D3D12VA implementation may share the same dpb
>> logic and some other functions. A base layer encode context is introduced
>> as vaapi context's base and extract the related funtions to a common
>> file such as receive_packet, init, close, etc.
>>
>> Signed-off-by: Tong Wu 
>> ---
>>   libavcodec/Makefile |   2 +-
>>   libavcodec/hw_base_encode.c | 637 ++
>>   libavcodec/hw_base_encode.h | 277 ++
>>   libavcodec/vaapi_encode.c   | 924 +++-
>>   libavcodec/vaapi_encode.h   | 229 +---
>>   libavcodec/vaapi_encode_av1.c   |  73 +--
>>   libavcodec/vaapi_encode_h264.c  | 201 +++
>>   libavcodec/vaapi_encode_h265.c  | 163 +++---
>>   libavcodec/vaapi_encode_mjpeg.c |  22 +-
>>   libavcodec/vaapi_encode_mpeg2.c |  53 +-
>>   libavcodec/vaapi_encode_vp8.c   |  28 +-
>>   libavcodec/vaapi_encode_vp9.c   |  70 +--
>>   12 files changed, 1427 insertions(+), 1252 deletions(-)
>>   create mode 100644 libavcodec/hw_base_encode.c
>>   create mode 100644 libavcodec/hw_base_encode.h
>>
>> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
>> index 470d7cb9b1..23946f6ea3 100644
>> --- a/libavcodec/Makefile
>> +++ b/libavcodec/Makefile
>> @@ -164,7 +164,7 @@ OBJS-$(CONFIG_STARTCODE)   += startcode.o
>>   OBJS-$(CONFIG_TEXTUREDSP)  += texturedsp.o
>>   OBJS-$(CONFIG_TEXTUREDSPENC)   += texturedspenc.o
>>   OBJS-$(CONFIG_TPELDSP) += tpeldsp.o
>> -OBJS-$(CONFIG_VAAPI_ENCODE)+= vaapi_encode.o
>> +OBJS-$(CONFIG_VAAPI_ENCODE)+= vaapi_encode.o
>hw_base_encode.o
>>   OBJS-$(CONFIG_AV1_AMF_ENCODER) += amfenc_av1.o
>>   OBJS-$(CONFIG_VC1DSP)  += vc1dsp.o
>>   OBJS-$(CONFIG_VIDEODSP)+= videodsp.o
>> diff --git a/libavcodec/hw_base_encode.c b/libavcodec/hw_base_encode.c
>> ...
>
>I'm going to trust that the contents of this file are only moved around with no
>functional changes.  If that isn't the case please do say.

You are absolutely right.

>
>> diff --git a/libavcodec/hw_base_encode.h b/libavcodec/hw_base_encode.h
>> new file mode 100644
>> index 00..70982c97b2
>> --- /dev/null
>> +++ b/libavcodec/hw_base_encode.h
>> @@ -0,0 +1,277 @@
>> +/*
>> + * Copyright (c) 2024 Intel Corporation
>
>I would avoid adding this.  Most of these files have content mostly copied from
>other files which are not exclusively copyright by Intel Corporation.

Will delete.

>
>> + *
>> + * 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
>> +
>> +#include "libavutil/hwcontext.h"
>> +#include "libavutil/fifo.h"
>> +
>> +#include "avcodec.h"
>> +#include "hwconfig.h"
>
>This file doesn't do anything with hwconfig stuff?
>

Will delete.

>> +
>> +enum {
>> +MAX_DPB_SIZE   = 16,
>> +MAX_PICTURE_REFERENCES = 2,
>> +MAX_REORDER_DELAY  = 16,
>> +MAX_ASYNC_DEPTH= 64,
>> +MAX_REFERENCE_LIST_NUM = 2,
>> +};
>> +
>> +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,
>> +// Codec output packet without timestamp delay, which means the
>> +// output packet has same PTS and DTS.
>> +FLAG_TIMESTAMP_NO_DELAY= 1 << 6,
>> +};
>> +
>> +enum {
>> +RC_MODE_AUTO,
>> +RC_MODE_CQP,
>>

Re: [FFmpeg-devel] [PATCH v5 2/9] avcodec/vaapi_encode: introduce a base layer for vaapi encode

2024-02-18 Thread Mark Thompson 

On 18/02/2024 08:45, tong1.wu-at-intel@ffmpeg.org wrote:

From: Tong Wu 

Since VAAPI and future D3D12VA implementation may share the same dpb
logic and some other functions. A base layer encode context is introduced
as vaapi context's base and extract the related funtions to a common
file such as receive_packet, init, close, etc.

Signed-off-by: Tong Wu 
---
  libavcodec/Makefile |   2 +-
  libavcodec/hw_base_encode.c | 637 ++
  libavcodec/hw_base_encode.h | 277 ++
  libavcodec/vaapi_encode.c   | 924 +++-
  libavcodec/vaapi_encode.h   | 229 +---
  libavcodec/vaapi_encode_av1.c   |  73 +--
  libavcodec/vaapi_encode_h264.c  | 201 +++
  libavcodec/vaapi_encode_h265.c  | 163 +++---
  libavcodec/vaapi_encode_mjpeg.c |  22 +-
  libavcodec/vaapi_encode_mpeg2.c |  53 +-
  libavcodec/vaapi_encode_vp8.c   |  28 +-
  libavcodec/vaapi_encode_vp9.c   |  70 +--
  12 files changed, 1427 insertions(+), 1252 deletions(-)
  create mode 100644 libavcodec/hw_base_encode.c
  create mode 100644 libavcodec/hw_base_encode.h

diff --git a/libavcodec/Makefile b/libavcodec/Makefile
index 470d7cb9b1..23946f6ea3 100644
--- a/libavcodec/Makefile
+++ b/libavcodec/Makefile
@@ -164,7 +164,7 @@ OBJS-$(CONFIG_STARTCODE)   += startcode.o
  OBJS-$(CONFIG_TEXTUREDSP)  += texturedsp.o
  OBJS-$(CONFIG_TEXTUREDSPENC)   += texturedspenc.o
  OBJS-$(CONFIG_TPELDSP) += tpeldsp.o
-OBJS-$(CONFIG_VAAPI_ENCODE)+= vaapi_encode.o
+OBJS-$(CONFIG_VAAPI_ENCODE)+= vaapi_encode.o hw_base_encode.o
  OBJS-$(CONFIG_AV1_AMF_ENCODER) += amfenc_av1.o
  OBJS-$(CONFIG_VC1DSP)  += vc1dsp.o
  OBJS-$(CONFIG_VIDEODSP)+= videodsp.o
diff --git a/libavcodec/hw_base_encode.c b/libavcodec/hw_base_encode.c
...


I'm going to trust that the contents of this file are only moved around with no 
functional changes.  If that isn't the case please do say.


diff --git a/libavcodec/hw_base_encode.h b/libavcodec/hw_base_encode.h
new file mode 100644
index 00..70982c97b2
--- /dev/null
+++ b/libavcodec/hw_base_encode.h
@@ -0,0 +1,277 @@
+/*
+ * Copyright (c) 2024 Intel Corporation


I would avoid adding this.  Most of these files have content mostly copied from 
other files which are not exclusively copyright by Intel Corporation.


+ *
+ * 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
+
+#include "libavutil/hwcontext.h"
+#include "libavutil/fifo.h"
+
+#include "avcodec.h"
+#include "hwconfig.h"


This file doesn't do anything with hwconfig stuff?


+
+enum {
+MAX_DPB_SIZE   = 16,
+MAX_PICTURE_REFERENCES = 2,
+MAX_REORDER_DELAY  = 16,
+MAX_ASYNC_DEPTH= 64,
+MAX_REFERENCE_LIST_NUM = 2,
+};
+
+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,
+// Codec output packet without timestamp delay, which means the
+// output packet has same PTS and DTS.
+FLAG_TIMESTAMP_NO_DELAY= 1 << 6,
+};
+
+enum {
+RC_MODE_AUTO,
+RC_MODE_CQP,
+RC_MODE_CBR,
+RC_MODE_VBR,
+RC_MODE_ICQ,


I thought ICQ was an Intel name which leaked into libva?  This probably 
shouldn't be in a generic API, maybe something about constant-quality.


+RC_MODE_QVBR,
+RC_MODE_AVBR,


More generally, I think you want to document what these modes are intended to be.  When 
they mapped directly to VAAPI then it was clear that the responsibility was 
"whatever libva gives you", but