Re: [FFmpeg-devel] [PATCH 6/6] avcodec/qsvdec: Implement SEI parsing for QSV decoders
> -Original Message-
> From: Xiang, Haihao
> Sent: Wednesday, June 1, 2022 7:16 AM
> To: ffmpeg-devel@ffmpeg.org
> Cc: softwo...@hotmail.com
> Subject: Re: [FFmpeg-devel] [PATCH 6/6] avcodec/qsvdec: Implement SEI
> parsing for QSV decoders
>
> On Thu, 2022-05-26 at 08:08 +, softworkz wrote:
> > From: softworkz
> >
> > Signed-off-by: softworkz
> > ---
> > libavcodec/qsvdec.c | 233
>
> > 1 file changed, 233 insertions(+)
> >
> > diff --git a/libavcodec/qsvdec.c b/libavcodec/qsvdec.c
> > index 5fc5bed4c8..7d6a491aa0 100644
> > --- a/libavcodec/qsvdec.c
> > +++ b/libavcodec/qsvdec.c
> > @@ -49,6 +49,12 @@
> > #include "hwconfig.h"
> > #include "qsv.h"
> > #include "qsv_internal.h"
> > +#include "h264dec.h"
> > +#include "h264_sei.h"
> > +#include "hevcdec.h"
> > +#include "hevc_ps.h"
> > +#include "hevc_sei.h"
> > +#include "mpeg12.h"
> >
> > static const AVRational mfx_tb = { 1, 9 };
> >
> > @@ -60,6 +66,8 @@ static const AVRational mfx_tb = { 1, 9 };
> > AV_NOPTS_VALUE : pts_tb.num ? \
> > av_rescale_q(mfx_pts, mfx_tb, pts_tb) : mfx_pts)
> >
> > +#define PAYLOAD_BUFFER_SIZE 65535
> > +
> > typedef struct QSVAsyncFrame {
> > mfxSyncPoint *sync;
> > QSVFrame *frame;
> > @@ -101,6 +109,9 @@ typedef struct QSVContext {
> >
> > mfxExtBuffer **ext_buffers;
> > int nb_ext_buffers;
> > +
> > +mfxU8 payload_buffer[PAYLOAD_BUFFER_SIZE];
> > +Mpeg1Context mpeg_ctx;
>
> I wonder why only mpeg1 context is required in QSVContext.
This is due to different implementations of SEI (user data
in case of mpeg) data decoding.
For H264 SEI decoding, we need H264SEIContext only.
see ff_h264_sei_decode()
Also, we don't need to state information between calls,
so we can have that as a stack variable in parse_sei_h264().
Similar applies to HEVC, where we use HEVCSEI and HEVCParamSets.
For MPEG1/2 video, we need to preserve state between some
user data parsing calls; for that reason it needs to be
a member of QsvContext while the others don't.
> > } QSVContext;
> >
> > static const AVCodecHWConfigInternal *const qsv_hw_configs[] = {
> > @@ -599,6 +610,208 @@ static int
> qsv_export_film_grain(AVCodecContext *avctx,
> > mfxExtAV1FilmGrainParam
> > return 0;
> > }
> > #endif
> > +static int find_start_offset(mfxU8 data[4])
> > +{
> > +if (data[0] == 0 && data[1] == 0 && data[2] == 1)
> > +return 3;
> > +
> > +if (data[0] == 0 && data[1] == 0 && data[2] == 0 && data[3] ==
> 1)
> > +return 4;
> > +
> > +return 0;
> > +}
> > +
> > +static int parse_sei_h264(AVCodecContext* avctx, QSVContext* q,
> AVFrame* out)
> > +{
> > +H264SEIContext sei = { 0 };
> > +GetBitContext gb = { 0 };
> > +mfxPayload payload = { 0, .Data = >payload_buffer[0],
> .BufSize =
> > sizeof(q->payload_buffer) };
> > +mfxU64 ts;
> > +int ret;
> > +
> > +while (1) {
> > +int start;
> > +memset(payload.Data, 0, payload.BufSize);
> > +
> > +ret = MFXVideoDECODE_GetPayload(q->session, ,
> );
> > +if (ret != MFX_ERR_NONE) {
> > +av_log(avctx, AV_LOG_WARNING, "error getting SEI
> payload: %d \n",
> > ret);
>
> Better to use AV_LOG_ERROR to match the description.
See answer below
(sorry, worked on it from bottom to top ;-)
>
> > +return ret;
> > +}
> > +
> > +if (payload.NumBit == 0 || payload.NumBit >=
> payload.BufSize * 8) {
> > +break;
> > +}
> > +
> > +start = find_start_offset(payload.Data);
> > +
> > +switch (payload.Type) {
> > +case SEI_TYPE_BUFFERING_PERIOD:
> > +case SEI_TYPE_PIC_TIMING:
> > +continue;
> > +}
> > +
> > +if (init_get_bits(, [start],
> payload.NumBit - start *
> > 8) < 0)
> > +av_log(avctx, AV_LOG_ERROR, "Error initializing
> bitstream
> > reader");
> > +else
>
> I think it should return an error when failed to initialize
> GetBitContext, and
> the `else` statement is not needed.
See answer below.
>
> > +ret = ff_h264_sei_
Re: [FFmpeg-devel] [PATCH 6/6] avcodec/qsvdec: Implement SEI parsing for QSV decoders
On Thu, 2022-05-26 at 08:08 +, softworkz wrote:
> From: softworkz
>
> Signed-off-by: softworkz
> ---
> libavcodec/qsvdec.c | 233
> 1 file changed, 233 insertions(+)
>
> diff --git a/libavcodec/qsvdec.c b/libavcodec/qsvdec.c
> index 5fc5bed4c8..7d6a491aa0 100644
> --- a/libavcodec/qsvdec.c
> +++ b/libavcodec/qsvdec.c
> @@ -49,6 +49,12 @@
> #include "hwconfig.h"
> #include "qsv.h"
> #include "qsv_internal.h"
> +#include "h264dec.h"
> +#include "h264_sei.h"
> +#include "hevcdec.h"
> +#include "hevc_ps.h"
> +#include "hevc_sei.h"
> +#include "mpeg12.h"
>
> static const AVRational mfx_tb = { 1, 9 };
>
> @@ -60,6 +66,8 @@ static const AVRational mfx_tb = { 1, 9 };
> AV_NOPTS_VALUE : pts_tb.num ? \
> av_rescale_q(mfx_pts, mfx_tb, pts_tb) : mfx_pts)
>
> +#define PAYLOAD_BUFFER_SIZE 65535
> +
> typedef struct QSVAsyncFrame {
> mfxSyncPoint *sync;
> QSVFrame *frame;
> @@ -101,6 +109,9 @@ typedef struct QSVContext {
>
> mfxExtBuffer **ext_buffers;
> int nb_ext_buffers;
> +
> +mfxU8 payload_buffer[PAYLOAD_BUFFER_SIZE];
> +Mpeg1Context mpeg_ctx;
I wonder why only mpeg1 context is required in QSVContext.
> } QSVContext;
>
> static const AVCodecHWConfigInternal *const qsv_hw_configs[] = {
> @@ -599,6 +610,208 @@ static int qsv_export_film_grain(AVCodecContext *avctx,
> mfxExtAV1FilmGrainParam
> return 0;
> }
> #endif
> +static int find_start_offset(mfxU8 data[4])
> +{
> +if (data[0] == 0 && data[1] == 0 && data[2] == 1)
> +return 3;
> +
> +if (data[0] == 0 && data[1] == 0 && data[2] == 0 && data[3] == 1)
> +return 4;
> +
> +return 0;
> +}
> +
> +static int parse_sei_h264(AVCodecContext* avctx, QSVContext* q, AVFrame* out)
> +{
> +H264SEIContext sei = { 0 };
> +GetBitContext gb = { 0 };
> +mfxPayload payload = { 0, .Data = >payload_buffer[0], .BufSize =
> sizeof(q->payload_buffer) };
> +mfxU64 ts;
> +int ret;
> +
> +while (1) {
> +int start;
> +memset(payload.Data, 0, payload.BufSize);
> +
> +ret = MFXVideoDECODE_GetPayload(q->session, , );
> +if (ret != MFX_ERR_NONE) {
> +av_log(avctx, AV_LOG_WARNING, "error getting SEI payload: %d \n",
> ret);
Better to use AV_LOG_ERROR to match the description.
> +return ret;
> +}
> +
> +if (payload.NumBit == 0 || payload.NumBit >= payload.BufSize * 8) {
> +break;
> +}
> +
> +start = find_start_offset(payload.Data);
> +
> +switch (payload.Type) {
> +case SEI_TYPE_BUFFERING_PERIOD:
> +case SEI_TYPE_PIC_TIMING:
> +continue;
> +}
> +
> +if (init_get_bits(, [start], payload.NumBit - start *
> 8) < 0)
> +av_log(avctx, AV_LOG_ERROR, "Error initializing bitstream
> reader");
> +else
I think it should return an error when failed to initialize GetBitContext, and
the `else` statement is not needed.
> +ret = ff_h264_sei_decode(, , NULL, avctx);
> +
> +if (ret < 0)
> +av_log(avctx, AV_LOG_WARNING, "Error parsing SEI type:
> %d Numbits %d error: %d\n", payload.Type, payload.NumBit, ret);
Better to use AV_LOG_ERROR and return an error. Otherwise please use 'warning'
instead of 'error' in the description.
> +else
> +av_log(avctx, AV_LOG_DEBUG, "mfxPayload Type: %d Numbits %d\n",
> payload.Type, payload.NumBit);
> +}
> +
> +if (out)
> +return ff_h264_export_frame_props(avctx, , NULL, out);
> +
> +return 0;
> +}
> +
> +static int parse_sei_hevc(AVCodecContext* avctx, QSVContext* q, QSVFrame*
> out)
> +{
> +HEVCSEI sei = { 0 };
> +HEVCParamSets ps = { 0 };
> +GetBitContext gb = { 0 };
> +mfxPayload payload = { 0, .Data = >payload_buffer[0], .BufSize =
> sizeof(q->payload_buffer) };
> +mfxFrameSurface1 *surface = >surface;
> +mfxU64 ts;
> +int ret, has_logged = 0;
> +
> +while (1) {
> +int start;
> +memset(payload.Data, 0, payload.BufSize);
> +
> +ret = MFXVideoDECODE_GetPayload(q->session, , );
> +if (ret != MFX_ERR_NONE) {
> +av_log(avctx, AV_LOG_WARNING, "error getting SEI payload: %d \n",
> ret);
> +return 0;
It returns an error in parse_sei_h264() when MFXVideoDECODE_GetPayload fails to
get the payload. Please make the behavior consistent across the codecs.
(I'm fine to return 0 instead of an error to ignore errors in SEI decoding. )
> +}
> +
> +if (payload.NumBit == 0 || payload.NumBit >= payload.BufSize * 8) {
> +break;
> +}
> +
> +if (!has_logged) {
> +has_logged = 1;
> +av_log(avctx, AV_LOG_VERBOSE, "
> -\n");
> +av_log(avctx, AV_LOG_VERBOSE, "Start reading SEI - payload
> timestamp: %llu - surface timestamp: %llu\n", ts,
[FFmpeg-devel] [PATCH 6/6] avcodec/qsvdec: Implement SEI parsing for QSV decoders
From: softworkz
Signed-off-by: softworkz
---
libavcodec/qsvdec.c | 233
1 file changed, 233 insertions(+)
diff --git a/libavcodec/qsvdec.c b/libavcodec/qsvdec.c
index 5fc5bed4c8..7d6a491aa0 100644
--- a/libavcodec/qsvdec.c
+++ b/libavcodec/qsvdec.c
@@ -49,6 +49,12 @@
#include "hwconfig.h"
#include "qsv.h"
#include "qsv_internal.h"
+#include "h264dec.h"
+#include "h264_sei.h"
+#include "hevcdec.h"
+#include "hevc_ps.h"
+#include "hevc_sei.h"
+#include "mpeg12.h"
static const AVRational mfx_tb = { 1, 9 };
@@ -60,6 +66,8 @@ static const AVRational mfx_tb = { 1, 9 };
AV_NOPTS_VALUE : pts_tb.num ? \
av_rescale_q(mfx_pts, mfx_tb, pts_tb) : mfx_pts)
+#define PAYLOAD_BUFFER_SIZE 65535
+
typedef struct QSVAsyncFrame {
mfxSyncPoint *sync;
QSVFrame *frame;
@@ -101,6 +109,9 @@ typedef struct QSVContext {
mfxExtBuffer **ext_buffers;
int nb_ext_buffers;
+
+mfxU8 payload_buffer[PAYLOAD_BUFFER_SIZE];
+Mpeg1Context mpeg_ctx;
} QSVContext;
static const AVCodecHWConfigInternal *const qsv_hw_configs[] = {
@@ -599,6 +610,208 @@ static int qsv_export_film_grain(AVCodecContext *avctx,
mfxExtAV1FilmGrainParam
return 0;
}
#endif
+static int find_start_offset(mfxU8 data[4])
+{
+if (data[0] == 0 && data[1] == 0 && data[2] == 1)
+return 3;
+
+if (data[0] == 0 && data[1] == 0 && data[2] == 0 && data[3] == 1)
+return 4;
+
+return 0;
+}
+
+static int parse_sei_h264(AVCodecContext* avctx, QSVContext* q, AVFrame* out)
+{
+H264SEIContext sei = { 0 };
+GetBitContext gb = { 0 };
+mfxPayload payload = { 0, .Data = >payload_buffer[0], .BufSize =
sizeof(q->payload_buffer) };
+mfxU64 ts;
+int ret;
+
+while (1) {
+int start;
+memset(payload.Data, 0, payload.BufSize);
+
+ret = MFXVideoDECODE_GetPayload(q->session, , );
+if (ret != MFX_ERR_NONE) {
+av_log(avctx, AV_LOG_WARNING, "error getting SEI payload: %d \n",
ret);
+return ret;
+}
+
+if (payload.NumBit == 0 || payload.NumBit >= payload.BufSize * 8) {
+break;
+}
+
+start = find_start_offset(payload.Data);
+
+switch (payload.Type) {
+case SEI_TYPE_BUFFERING_PERIOD:
+case SEI_TYPE_PIC_TIMING:
+continue;
+}
+
+if (init_get_bits(, [start], payload.NumBit - start *
8) < 0)
+av_log(avctx, AV_LOG_ERROR, "Error initializing bitstream reader");
+else
+ret = ff_h264_sei_decode(, , NULL, avctx);
+
+if (ret < 0)
+av_log(avctx, AV_LOG_WARNING, "Error parsing SEI type: %d Numbits
%d error: %d\n", payload.Type, payload.NumBit, ret);
+else
+av_log(avctx, AV_LOG_DEBUG, "mfxPayload Type: %d Numbits %d\n",
payload.Type, payload.NumBit);
+}
+
+if (out)
+return ff_h264_export_frame_props(avctx, , NULL, out);
+
+return 0;
+}
+
+static int parse_sei_hevc(AVCodecContext* avctx, QSVContext* q, QSVFrame* out)
+{
+HEVCSEI sei = { 0 };
+HEVCParamSets ps = { 0 };
+GetBitContext gb = { 0 };
+mfxPayload payload = { 0, .Data = >payload_buffer[0], .BufSize =
sizeof(q->payload_buffer) };
+mfxFrameSurface1 *surface = >surface;
+mfxU64 ts;
+int ret, has_logged = 0;
+
+while (1) {
+int start;
+memset(payload.Data, 0, payload.BufSize);
+
+ret = MFXVideoDECODE_GetPayload(q->session, , );
+if (ret != MFX_ERR_NONE) {
+av_log(avctx, AV_LOG_WARNING, "error getting SEI payload: %d \n",
ret);
+return 0;
+}
+
+if (payload.NumBit == 0 || payload.NumBit >= payload.BufSize * 8) {
+break;
+}
+
+if (!has_logged) {
+has_logged = 1;
+av_log(avctx, AV_LOG_VERBOSE,
"-\n");
+av_log(avctx, AV_LOG_VERBOSE, "Start reading SEI - payload
timestamp: %llu - surface timestamp: %llu\n", ts, surface->Data.TimeStamp);
+}
+
+if (ts != surface->Data.TimeStamp) {
+av_log(avctx, AV_LOG_WARNING, "GetPayload timestamp (%llu) does
not match surface timestamp: (%llu)\n", ts, surface->Data.TimeStamp);
+}
+
+start = find_start_offset(payload.Data);
+
+av_log(avctx, AV_LOG_VERBOSE, "parsing SEI type: %3d Numbits %3d
Start: %d\n", payload.Type, payload.NumBit, start);
+
+switch (payload.Type) {
+case SEI_TYPE_BUFFERING_PERIOD:
+case SEI_TYPE_PIC_TIMING:
+continue;
+case SEI_TYPE_MASTERING_DISPLAY_COLOUR_VOLUME:
+// There seems to be a bug in MSDK
+payload.NumBit -= 8;
+
+break;
+case SEI_TYPE_CONTENT_LIGHT_LEVEL_INFO:
+// There seems to be a bug in MSDK
+payload.NumBit = 48;
+
+break;
+case
