Re: [FFmpeg-devel] [PATCH 6/6] avcodec/qsvdec: Implement SEI parsing for QSV decoders

[Soft Works <softworkz@hotmail.com>]

> -----Original Message-----
> From: Xiang, Haihao <haihao.xiang@intel.com>
> Sent: Wednesday, June 1, 2022 7:16 AM
> To: ffmpeg-devel@ffmpeg.org
> Cc: softworkz@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 +0000, softworkz wrote:
> > From: softworkz <softworkz@hotmail.com>
> >
> > Signed-off-by: softworkz <softworkz@hotmail.com>
> > ---
> >  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, 90000 };
> >
> > @@ -60,6 +66,8 @@ static const AVRational mfx_tb = { 1, 90000 };
> >      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 = &q->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, &ts,
> &payload);
> > +        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(&gb, &payload.Data[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_decode(&sei, &gb, 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.

I changed it as "Failed to parse SEI type:..."
and kept AV_LOG_WARNING


> > +        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, &sei, 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 = &q->payload_buffer[0],
> .BufSize =
> > sizeof(q->payload_buffer) };
> > +    mfxFrameSurface1 *surface = &out->surface;
> > +    mfxU64 ts;
> > +    int ret, has_logged = 0;
> > +
> > +    while (1) {
> > +        int start;
> > +        memset(payload.Data, 0, payload.BufSize);
> > +
> > +        ret = MFXVideoDECODE_GetPayload(q->session, &ts,
> &payload);
> > +        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. )

I made it consistent and added a special message for the typical
error (MFX_ERR_NOT_ENOUGH_BUFFER), even though it is unlikely to
happen, now that I've chosen a fairly large fixed-size buffer which
is part of the context.


> > +        }
> > +
> > +        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 SEI_TYPE_USER_DATA_REGISTERED_ITU_T_T35:
> > +                // There seems to be a bug in MSDK
> > +                if (payload.NumBit == 552)
> > +                    payload.NumBit = 528;
> > +                break;
> > +        }
> > +
> > +        if (init_get_bits(&gb, &payload.Data[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.

The output from MSDK cannot be 100% trusted as we have seen in the
recent discussion with the MSDK team.
In case of "normal" bit streams we would of course need to error
out, but in this implementation we always get a new buffer while
looping, so when there's an error with one of the buffers, we 
can still continue the loop and get the next buffer.

But I changed the code flow now to make it more clear and output
just a single error message when init_get_bits() would fail.


> > +            ret = ff_hevc_decode_nal_sei(&gb, avctx, &sei, &ps,
> > HEVC_NAL_SEI_PREFIX);
> > +
> > +        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 (has_logged) {
> > +        av_log(avctx, AV_LOG_VERBOSE, "End reading SEI\n");
> > +    }
> > +
> > +    if (out && out->frame)
> > +        return ff_set_side_data(avctx, &sei, NULL, out->frame);
> > +
> > +    return 0;
> > +}
> > +
> > +static int parse_sei_mpeg12(AVCodecContext* avctx, QSVContext* q,
> AVFrame*
> > out)
> > +{
> > +    Mpeg1Context *mpeg_ctx = &q->mpeg_ctx;
> > +    mfxPayload payload = { 0, .Data = &q->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, &ts,
> &payload);
> > +        if (ret != MFX_ERR_NONE) {
> > +            av_log(avctx, AV_LOG_WARNING, "error getting SEI
> payload: %d \n",
> > ret);
> 
> WARNING or ERROR ?

Fixed.

> 
> > +            return ret;
> > +        }
> > +
> > +        if (payload.NumBit == 0 || payload.NumBit >=
> payload.BufSize * 8) {
> > +            break;
> > +        }
> > +
> > +        start = find_start_offset(payload.Data);
> > +
> > +        start++;
> > +
> > +        ff_mpeg_decode_user_data(avctx, mpeg_ctx,
> &payload.Data[start],
> > (int)((payload.NumBit + 7) / 8) - start);
> > +
> > +        if (ret < 0)
> 
> Here ret is always MFX_ERR_NONE

Right. Dropped.

> 
> > +            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
> > start %d -> %.s\n", payload.Type, payload.NumBit, start, (char
> > *)(&payload.Data[start]));
> > +    }
> > +
> > +    if (!out)
> > +        return 0;
> > +
> > +    if (mpeg_ctx->a53_buf_ref) {
> > +
> > +        AVFrameSideData *sd = av_frame_new_side_data_from_buf(out,
> > AV_FRAME_DATA_A53_CC, mpeg_ctx->a53_buf_ref);
> > +        if (!sd)
> > +            av_buffer_unref(&mpeg_ctx->a53_buf_ref);
> > +        mpeg_ctx->a53_buf_ref = NULL;
> > +    }
> > +
> > +    if (mpeg_ctx->has_stereo3d) {
> > +        AVStereo3D *stereo = av_stereo3d_create_side_data(out);
> > +        if (!stereo)
> > +            return AVERROR(ENOMEM);
> > +
> > +        *stereo = mpeg_ctx->stereo3d;
> > +        mpeg_ctx->has_stereo3d = 0;
> > +    }
> > +
> > +    if (mpeg_ctx->has_afd) {
> > +        AVFrameSideData *sd = av_frame_new_side_data(out,
> AV_FRAME_DATA_AFD,
> > 1);
> > +        if (!sd)
> > +            return AVERROR(ENOMEM);
> > +
> > +        *sd->data   = mpeg_ctx->afd;
> > +        mpeg_ctx->has_afd = 0;
> > +    }
> > +
> > +    return 0;
> > +}
> >
> >  static int qsv_decode(AVCodecContext *avctx, QSVContext *q,
> >                        AVFrame *frame, int *got_frame,
> > @@ -636,6 +849,8 @@ static int qsv_decode(AVCodecContext *avctx,
> QSVContext
> > *q,
> >                                                insurf, &outsurf,
> sync);
> >          if (ret == MFX_WRN_DEVICE_BUSY)
> >              av_usleep(500);
> > +        else if (avctx->codec_id == AV_CODEC_ID_MPEG1VIDEO ||
> avctx->codec_id
> > == AV_CODEC_ID_MPEG2VIDEO)
> 
> 
> We didn't add qsv decoder for AV_CODEC_ID_MPEG1VIDEO in qsvdec.c

The decoder (mpeg2_qsv) could still be selected from the
command line.


> > +            parse_sei_mpeg12(avctx, q, NULL);
> >
> >      } while (ret == MFX_WRN_DEVICE_BUSY || ret ==
> MFX_ERR_MORE_SURFACE);
> >
> > @@ -677,6 +892,24 @@ static int qsv_decode(AVCodecContext *avctx,
> QSVContext
> > *q,
> >              return AVERROR_BUG;
> >          }
> >
> > +        switch (avctx->codec_id) {
> > +        case AV_CODEC_ID_MPEG1VIDEO:
> > +        case AV_CODEC_ID_MPEG2VIDEO:
> 
> No support for AV_CODEC_ID_MPEG1VIDEO.

I wasn't sure about that due to the explicit mapping here:

https://github.com/ffstaging/FFmpeg/blob/f55c91497d4d16d393ae9c034bd3032a683802ca/libavcodec/qsv.c#L50-L52

I thought that maybe the mpeg2 decoder would be able to handle mp1video
as well, but I did a bit of research and it turns out that you're right
and that it's not supported.

Removed the MPEG1VIDEO constants.


Thanks a lot for your review,
will submit an update shortly.

Best,
softworkz


_______________________________________________
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".