mbox series

[FFmpeg-devel,v5,0/6] Implement SEI parsing for QSV decoders

Message ID pull.31.v5.ffstaging.FFmpeg.1656708534.ffmpegagent@gmail.com
Headers show
Series Implement SEI parsing for QSV decoders | expand

Message

Aman Karmani July 1, 2022, 8:48 p.m. UTC
Missing SEI information has always been a major drawback when using the QSV
decoders. I used to think that there's no chance to get at the data without
explicit implementation from the MSDK side (or doing something weird like
parsing in parallel). It turned out that there's a hardly known api method
that provides access to all SEI (h264/hevc) or user data (mpeg2video).

This allows to get things like closed captions, frame packing, display
orientation, HDR data (mastering display, content light level, etc.) without
having to rely on those data being provided by the MSDK as extended buffers.

The commit "Implement SEI parsing for QSV decoders" includes some hard-coded
workarounds for MSDK bugs which I reported:
https://github.com/Intel-Media-SDK/MediaSDK/issues/2597#issuecomment-1072795311

But that doesn't help. Those bugs exist and I'm sharing my workarounds,
which are empirically determined by testing a range of files. If someone is
interested, I can provide private access to a repository where we have been
testing this. Alternatively, I could also leave those workarounds out, and
just skip those SEI types.

In a previous version of this patchset, there was a concern that payload
data might need to be re-ordered. Meanwhile I have researched this carefully
and the conclusion is that this is not required.

My detailed analysis can be found here:
https://gist.github.com/softworkz/36c49586a8610813a32270ee3947a932

v4

 * add new dependencies in makefile Now, build still works when someone uses
   configure --disable-decoder=h264 --disable-decoder=hevc
   --disable-decoder=mpegvideo --disable-decoder=mpeg1video
   --disable-decoder=mpeg2video --enable-libmfx

v3

 * frame.h: clarify doc text for av_frame_copy_side_data()

v2

 * qsvdec: make error handling consistent and clear
 * qsvdec: remove AV_CODEC_ID_MPEG1VIDEO constants
 * hevcdec: rename function to ff_hevc_set_side_data(), add doc text

v3

 * qsvdec: fix c/p error

softworkz (6):
  avutil/frame: Add av_frame_copy_side_data() and
    av_frame_remove_all_side_data()
  avcodec/vpp_qsv: Copy side data from input to output frame
  avcodec/mpeg12dec: make mpeg_decode_user_data() accessible
  avcodec/hevcdec: make set_side_data() accessible
  avcodec/h264dec: make h264_export_frame_props() accessible
  avcodec/qsvdec: Implement SEI parsing for QSV decoders

 doc/APIchanges               |   4 +
 libavcodec/Makefile          |   6 +-
 libavcodec/h264_slice.c      |  98 ++++++++-------
 libavcodec/h264dec.h         |   2 +
 libavcodec/hevcdec.c         | 117 +++++++++---------
 libavcodec/hevcdec.h         |   9 ++
 libavcodec/hevcdsp.c         |   4 +
 libavcodec/mpeg12.h          |  28 +++++
 libavcodec/mpeg12dec.c       |  40 +-----
 libavcodec/qsvdec.c          | 234 +++++++++++++++++++++++++++++++++++
 libavfilter/qsvvpp.c         |   6 +
 libavfilter/vf_overlay_qsv.c |  19 ++-
 libavutil/frame.c            |  67 ++++++----
 libavutil/frame.h            |  32 +++++
 libavutil/version.h          |   2 +-
 15 files changed, 494 insertions(+), 174 deletions(-)


base-commit: 6a82412bf33108111eb3f63076fd5a51349ae114
Published-As: https://github.com/ffstaging/FFmpeg/releases/tag/pr-ffstaging-31%2Fsoftworkz%2Fsubmit_qsv_sei-v5
Fetch-It-Via: git fetch https://github.com/ffstaging/FFmpeg pr-ffstaging-31/softworkz/submit_qsv_sei-v5
Pull-Request: https://github.com/ffstaging/FFmpeg/pull/31

Range-diff vs v4:

 1:  7656477360 = 1:  7656477360 avutil/frame: Add av_frame_copy_side_data() and av_frame_remove_all_side_data()
 2:  06976606c5 = 2:  06976606c5 avcodec/vpp_qsv: Copy side data from input to output frame
 3:  320a8a535c = 3:  320a8a535c avcodec/mpeg12dec: make mpeg_decode_user_data() accessible
 4:  e58ad6564f = 4:  e58ad6564f avcodec/hevcdec: make set_side_data() accessible
 5:  a57bfaebb9 = 5:  4c0b6eb4cb avcodec/h264dec: make h264_export_frame_props() accessible
 6:  3f2588563e ! 6:  19bc00be4d avcodec/qsvdec: Implement SEI parsing for QSV decoders
     @@ Commit message
      
          Signed-off-by: softworkz <softworkz@hotmail.com>
      
     + ## libavcodec/Makefile ##
     +@@ libavcodec/Makefile: OBJS-$(CONFIG_MSS34DSP)                += mss34dsp.o
     + OBJS-$(CONFIG_PIXBLOCKDSP)             += pixblockdsp.o
     + OBJS-$(CONFIG_QPELDSP)                 += qpeldsp.o
     + OBJS-$(CONFIG_QSV)                     += qsv.o
     +-OBJS-$(CONFIG_QSVDEC)                  += qsvdec.o
     ++OBJS-$(CONFIG_QSVDEC)                  += qsvdec.o h264_slice.o h264_cabac.o h264_cavlc.o \
     ++                                          h264_direct.o h264_mb.o h264_picture.o h264_loopfilter.o \
     ++                                          h264dec.o h264_refs.o cabac.o hevcdec.o hevc_refs.o \
     ++										  hevc_filter.o hevc_cabac.o hevc_mvs.o hevcpred.o hevcdsp.o \
     ++										  h274.o dovi_rpu.o mpeg12dec.o
     + OBJS-$(CONFIG_QSVENC)                  += qsvenc.o
     + OBJS-$(CONFIG_RANGECODER)              += rangecoder.o
     + OBJS-$(CONFIG_RDFT)                    += rdft.o
     +
     + ## libavcodec/hevcdsp.c ##
     +@@
     +  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
     +  */
     + 
     ++#include "config_components.h"
     ++
     + #include "hevcdsp.h"
     + 
     + static const int8_t transform[32][32] = {
     +@@ libavcodec/hevcdsp.c: int i = 0;
     +         break;
     +     }
     + 
     ++#if CONFIG_HEVC_DECODER
     + #if ARCH_AARCH64
     +     ff_hevc_dsp_init_aarch64(hevcdsp, bit_depth);
     + #elif ARCH_ARM
     +@@ libavcodec/hevcdsp.c: int i = 0;
     + #elif ARCH_LOONGARCH
     +     ff_hevc_dsp_init_loongarch(hevcdsp, bit_depth);
     + #endif
     ++#endif
     + }
     +
       ## libavcodec/qsvdec.c ##
      @@
       #include "hwconfig.h"

Comments

Xiang, Haihao July 19, 2022, 6:55 a.m. UTC | #1
On Fri, 2022-07-01 at 20:48 +0000, ffmpegagent wrote:
> Missing SEI information has always been a major drawback when using the QSV
> decoders. I used to think that there's no chance to get at the data without
> explicit implementation from the MSDK side (or doing something weird like
> parsing in parallel). It turned out that there's a hardly known api method
> that provides access to all SEI (h264/hevc) or user data (mpeg2video).
> 
> This allows to get things like closed captions, frame packing, display
> orientation, HDR data (mastering display, content light level, etc.) without
> having to rely on those data being provided by the MSDK as extended buffers.
> 
> The commit "Implement SEI parsing for QSV decoders" includes some hard-coded
> workarounds for MSDK bugs which I reported:
> 
https://github.com/Intel-Media-SDK/MediaSDK/issues/2597#issuecomment-1072795311
> 
> But that doesn't help. Those bugs exist and I'm sharing my workarounds,
> which are empirically determined by testing a range of files. If someone is
> interested, I can provide private access to a repository where we have been
> testing this. Alternatively, I could also leave those workarounds out, and
> just skip those SEI types.
> 
> In a previous version of this patchset, there was a concern that payload
> data might need to be re-ordered. Meanwhile I have researched this carefully
> and the conclusion is that this is not required.
> 
> My detailed analysis can be found here:
> https://gist.github.com/softworkz/36c49586a8610813a32270ee3947a932
> 
> v4
> 
>  * add new dependencies in makefile Now, build still works when someone uses
>    configure --disable-decoder=h264 --disable-decoder=hevc
>    --disable-decoder=mpegvideo --disable-decoder=mpeg1video
>    --disable-decoder=mpeg2video --enable-libmfx
> 
> v3
> 
>  * frame.h: clarify doc text for av_frame_copy_side_data()
> 
> v2
> 
>  * qsvdec: make error handling consistent and clear
>  * qsvdec: remove AV_CODEC_ID_MPEG1VIDEO constants
>  * hevcdec: rename function to ff_hevc_set_side_data(), add doc text
> 
> v3
> 
>  * qsvdec: fix c/p error
> 
> softworkz (6):
>   avutil/frame: Add av_frame_copy_side_data() and
>     av_frame_remove_all_side_data()
>   avcodec/vpp_qsv: Copy side data from input to output frame
>   avcodec/mpeg12dec: make mpeg_decode_user_data() accessible
>   avcodec/hevcdec: make set_side_data() accessible
>   avcodec/h264dec: make h264_export_frame_props() accessible
>   avcodec/qsvdec: Implement SEI parsing for QSV decoders
> 
>  doc/APIchanges               |   4 +
>  libavcodec/Makefile          |   6 +-
>  libavcodec/h264_slice.c      |  98 ++++++++-------
>  libavcodec/h264dec.h         |   2 +
>  libavcodec/hevcdec.c         | 117 +++++++++---------
>  libavcodec/hevcdec.h         |   9 ++
>  libavcodec/hevcdsp.c         |   4 +
>  libavcodec/mpeg12.h          |  28 +++++
>  libavcodec/mpeg12dec.c       |  40 +-----
>  libavcodec/qsvdec.c          | 234 +++++++++++++++++++++++++++++++++++
>  libavfilter/qsvvpp.c         |   6 +
>  libavfilter/vf_overlay_qsv.c |  19 ++-
>  libavutil/frame.c            |  67 ++++++----
>  libavutil/frame.h            |  32 +++++
>  libavutil/version.h          |   2 +-
>  15 files changed, 494 insertions(+), 174 deletions(-)
> 
> 
> base-commit: 6a82412bf33108111eb3f63076fd5a51349ae114
> Published-As: 
> https://github.com/ffstaging/FFmpeg/releases/tag/pr-ffstaging-31%2Fsoftworkz%2Fsubmit_qsv_sei-v5
> Fetch-It-Via: git fetch https://github.com/ffstaging/FFmpeg pr-ffstaging-
> 31/softworkz/submit_qsv_sei-v5
> Pull-Request: https://github.com/ffstaging/FFmpeg/pull/31
> 
> Range-diff vs v4:
> 
>  1:  7656477360 = 1:  7656477360 avutil/frame: Add av_frame_copy_side_data()
> and av_frame_remove_all_side_data()
>  2:  06976606c5 = 2:  06976606c5 avcodec/vpp_qsv: Copy side data from input to
> output frame
>  3:  320a8a535c = 3:  320a8a535c avcodec/mpeg12dec: make
> mpeg_decode_user_data() accessible
>  4:  e58ad6564f = 4:  e58ad6564f avcodec/hevcdec: make set_side_data()
> accessible
>  5:  a57bfaebb9 = 5:  4c0b6eb4cb avcodec/h264dec: make
> h264_export_frame_props() accessible
>  6:  3f2588563e ! 6:  19bc00be4d avcodec/qsvdec: Implement SEI parsing for QSV
> decoders
>      @@ Commit message
>       
>           Signed-off-by: softworkz <softworkz@hotmail.com>
>       
>      + ## libavcodec/Makefile ##
>      +@@ libavcodec/Makefile: OBJS-$(CONFIG_MSS34DSP)                +=
> mss34dsp.o
>      + OBJS-$(CONFIG_PIXBLOCKDSP)             += pixblockdsp.o
>      + OBJS-$(CONFIG_QPELDSP)                 += qpeldsp.o
>      + OBJS-$(CONFIG_QSV)                     += qsv.o
>      +-OBJS-$(CONFIG_QSVDEC)                  += qsvdec.o
>      ++OBJS-$(CONFIG_QSVDEC)                  += qsvdec.o h264_slice.o
> h264_cabac.o h264_cavlc.o \
>      ++                                          h264_direct.o h264_mb.o
> h264_picture.o h264_loopfilter.o \
>      ++                                          h264dec.o h264_refs.o cabac.o
> hevcdec.o hevc_refs.o \
>      ++										
>   hevc_filter.o hevc_cabac.o hevc_mvs.o hevcpred.o hevcdsp.o \
>      ++										
>   h274.o dovi_rpu.o mpeg12dec.o
>      + OBJS-$(CONFIG_QSVENC)                  += qsvenc.o
>      + OBJS-$(CONFIG_RANGECODER)              += rangecoder.o
>      + OBJS-$(CONFIG_RDFT)                    += rdft.o
>      +
>      + ## libavcodec/hevcdsp.c ##
>      +@@
>      +  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-
> 1301 USA
>      +  */
>      + 
>      ++#include "config_components.h"
>      ++
>      + #include "hevcdsp.h"
>      + 
>      + static const int8_t transform[32][32] = {
>      +@@ libavcodec/hevcdsp.c: int i = 0;
>      +         break;
>      +     }
>      + 
>      ++#if CONFIG_HEVC_DECODER
>      + #if ARCH_AARCH64
>      +     ff_hevc_dsp_init_aarch64(hevcdsp, bit_depth);
>      + #elif ARCH_ARM
>      +@@ libavcodec/hevcdsp.c: int i = 0;
>      + #elif ARCH_LOONGARCH
>      +     ff_hevc_dsp_init_loongarch(hevcdsp, bit_depth);
>      + #endif
>      ++#endif
>      + }
>      +
>        ## libavcodec/qsvdec.c ##
>       @@
>        #include "hwconfig.h"


Is there any comment on this patchset ? If not, I'd like to merge it to make QSV
decoders works with SEI info.

Thanks
Haihao


>
Soft Works July 21, 2022, 9:06 p.m. UTC | #2
> -----Original Message-----
> From: Xiang, Haihao <haihao.xiang@intel.com>
> Sent: Tuesday, July 19, 2022 8:55 AM
> To: ffmpeg-devel@ffmpeg.org
> Cc: andreas.rheinhardt@outlook.com; kierank@obe.tv; haihao.xiang-at-
> intel.com@ffmpeg.org; softworkz@hotmail.com
> Subject: Re: [FFmpeg-devel] [PATCH v5 0/6] Implement SEI parsing for
> QSV decoders
> 
> On Fri, 2022-07-01 at 20:48 +0000, ffmpegagent wrote:
> > Missing SEI information has always been a major drawback when using
> the QSV
> > decoders. I used to think that there's no chance to get at the data
> without
> > explicit implementation from the MSDK side (or doing something
> weird like
> > parsing in parallel). It turned out that there's a hardly known api
> method
> > that provides access to all SEI (h264/hevc) or user data
> (mpeg2video).
> >
> > This allows to get things like closed captions, frame packing,
> display
> > orientation, HDR data (mastering display, content light level,
> etc.) without
> > having to rely on those data being provided by the MSDK as extended
> buffers.
> >
> > The commit "Implement SEI parsing for QSV decoders" includes some
> hard-coded
> > workarounds for MSDK bugs which I reported:
> >
> https://github.com/Intel-Media-SDK/MediaSDK/issues/2597#issuecomment-
> 1072795311
> >
> > But that doesn't help. Those bugs exist and I'm sharing my
> workarounds,
> > which are empirically determined by testing a range of files. If
> someone is
> > interested, I can provide private access to a repository where we
> have been
> > testing this. Alternatively, I could also leave those workarounds
> out, and
> > just skip those SEI types.
> >
> > In a previous version of this patchset, there was a concern that
> payload
> > data might need to be re-ordered. Meanwhile I have researched this
> carefully
> > and the conclusion is that this is not required.
> >
> > My detailed analysis can be found here:
> > https://gist.github.com/softworkz/36c49586a8610813a32270ee3947a932
> >
> > v4
> >
> >  * add new dependencies in makefile Now, build still works when
> someone uses
> >    configure --disable-decoder=h264 --disable-decoder=hevc
> >    --disable-decoder=mpegvideo --disable-decoder=mpeg1video
> >    --disable-decoder=mpeg2video --enable-libmfx
> >
> > v3
> >
> >  * frame.h: clarify doc text for av_frame_copy_side_data()
> >
> > v2
> >
> >  * qsvdec: make error handling consistent and clear
> >  * qsvdec: remove AV_CODEC_ID_MPEG1VIDEO constants
> >  * hevcdec: rename function to ff_hevc_set_side_data(), add doc
> text
> >
> > v3
> >
> >  * qsvdec: fix c/p error
> >
> > softworkz (6):
> >   avutil/frame: Add av_frame_copy_side_data() and
> >     av_frame_remove_all_side_data()
> >   avcodec/vpp_qsv: Copy side data from input to output frame
> >   avcodec/mpeg12dec: make mpeg_decode_user_data() accessible
> >   avcodec/hevcdec: make set_side_data() accessible
> >   avcodec/h264dec: make h264_export_frame_props() accessible
> >   avcodec/qsvdec: Implement SEI parsing for QSV decoders
> >
> >  doc/APIchanges               |   4 +
> >  libavcodec/Makefile          |   6 +-
> >  libavcodec/h264_slice.c      |  98 ++++++++-------
> >  libavcodec/h264dec.h         |   2 +
> >  libavcodec/hevcdec.c         | 117 +++++++++---------
> >  libavcodec/hevcdec.h         |   9 ++
> >  libavcodec/hevcdsp.c         |   4 +
> >  libavcodec/mpeg12.h          |  28 +++++
> >  libavcodec/mpeg12dec.c       |  40 +-----
> >  libavcodec/qsvdec.c          | 234
> +++++++++++++++++++++++++++++++++++
> >  libavfilter/qsvvpp.c         |   6 +
> >  libavfilter/vf_overlay_qsv.c |  19 ++-
> >  libavutil/frame.c            |  67 ++++++----
> >  libavutil/frame.h            |  32 +++++
> >  libavutil/version.h          |   2 +-
> >  15 files changed, 494 insertions(+), 174 deletions(-)
> >
> >
> > base-commit: 6a82412bf33108111eb3f63076fd5a51349ae114
> > Published-As:
> > https://github.com/ffstaging/FFmpeg/releases/tag/pr-ffstaging-
> 31%2Fsoftworkz%2Fsubmit_qsv_sei-v5
> > Fetch-It-Via: git fetch https://github.com/ffstaging/FFmpeg pr-
> ffstaging-
> > 31/softworkz/submit_qsv_sei-v5
> > Pull-Request: https://github.com/ffstaging/FFmpeg/pull/31
> >
> > Range-diff vs v4:
> >
> >  1:  7656477360 = 1:  7656477360 avutil/frame: Add
> av_frame_copy_side_data()
> > and av_frame_remove_all_side_data()
> >  2:  06976606c5 = 2:  06976606c5 avcodec/vpp_qsv: Copy side data
> from input to
> > output frame
> >  3:  320a8a535c = 3:  320a8a535c avcodec/mpeg12dec: make
> > mpeg_decode_user_data() accessible
> >  4:  e58ad6564f = 4:  e58ad6564f avcodec/hevcdec: make
> set_side_data()
> > accessible
> >  5:  a57bfaebb9 = 5:  4c0b6eb4cb avcodec/h264dec: make
> > h264_export_frame_props() accessible
> >  6:  3f2588563e ! 6:  19bc00be4d avcodec/qsvdec: Implement SEI
> parsing for QSV
> > decoders
> >      @@ Commit message
> >
> >           Signed-off-by: softworkz <softworkz@hotmail.com>
> >
> >      + ## libavcodec/Makefile ##
> >      +@@ libavcodec/Makefile: OBJS-$(CONFIG_MSS34DSP)
> +=
> > mss34dsp.o
> >      + OBJS-$(CONFIG_PIXBLOCKDSP)             += pixblockdsp.o
> >      + OBJS-$(CONFIG_QPELDSP)                 += qpeldsp.o
> >      + OBJS-$(CONFIG_QSV)                     += qsv.o
> >      +-OBJS-$(CONFIG_QSVDEC)                  += qsvdec.o
> >      ++OBJS-$(CONFIG_QSVDEC)                  += qsvdec.o
> h264_slice.o
> > h264_cabac.o h264_cavlc.o \
> >      ++                                          h264_direct.o
> h264_mb.o
> > h264_picture.o h264_loopfilter.o \
> >      ++                                          h264dec.o
> h264_refs.o cabac.o
> > hevcdec.o hevc_refs.o \
> >      ++
> 
> >   hevc_filter.o hevc_cabac.o hevc_mvs.o hevcpred.o hevcdsp.o \
> >      ++
> 
> >   h274.o dovi_rpu.o mpeg12dec.o
> >      + OBJS-$(CONFIG_QSVENC)                  += qsvenc.o
> >      + OBJS-$(CONFIG_RANGECODER)              += rangecoder.o
> >      + OBJS-$(CONFIG_RDFT)                    += rdft.o
> >      +
> >      + ## libavcodec/hevcdsp.c ##
> >      +@@
> >      +  * Foundation, Inc., 51 Franklin Street, Fifth Floor,
> Boston, MA 02110-
> > 1301 USA
> >      +  */
> >      +
> >      ++#include "config_components.h"
> >      ++
> >      + #include "hevcdsp.h"
> >      +
> >      + static const int8_t transform[32][32] = {
> >      +@@ libavcodec/hevcdsp.c: int i = 0;
> >      +         break;
> >      +     }
> >      +
> >      ++#if CONFIG_HEVC_DECODER
> >      + #if ARCH_AARCH64
> >      +     ff_hevc_dsp_init_aarch64(hevcdsp, bit_depth);
> >      + #elif ARCH_ARM
> >      +@@ libavcodec/hevcdsp.c: int i = 0;
> >      + #elif ARCH_LOONGARCH
> >      +     ff_hevc_dsp_init_loongarch(hevcdsp, bit_depth);
> >      + #endif
> >      ++#endif
> >      + }
> >      +
> >        ## libavcodec/qsvdec.c ##
> >       @@
> >        #include "hwconfig.h"
> 
> 
> Is there any comment on this patchset ? If not, I'd like to merge it
> to make QSV
> decoders works with SEI info.
> 
> Thanks
> Haihao


There's a conflicting (but reasonable and useful) patchset from Andreas:

https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=6959


Even though this is open for so long already, it think it makes more
sense to merge Andreas' first and adapt mine to be applied on top
of it.

@Andreas - any news on that patchset of yours?

Thanks,
softworkz
Andreas Rheinhardt July 21, 2022, 9:56 p.m. UTC | #3
Xiang, Haihao:
> On Fri, 2022-07-01 at 20:48 +0000, ffmpegagent wrote:
>> Missing SEI information has always been a major drawback when using the QSV
>> decoders. I used to think that there's no chance to get at the data without
>> explicit implementation from the MSDK side (or doing something weird like
>> parsing in parallel). It turned out that there's a hardly known api method
>> that provides access to all SEI (h264/hevc) or user data (mpeg2video).
>>
>> This allows to get things like closed captions, frame packing, display
>> orientation, HDR data (mastering display, content light level, etc.) without
>> having to rely on those data being provided by the MSDK as extended buffers.
>>
>> The commit "Implement SEI parsing for QSV decoders" includes some hard-coded
>> workarounds for MSDK bugs which I reported:
>>
> https://github.com/Intel-Media-SDK/MediaSDK/issues/2597#issuecomment-1072795311
>>
>> But that doesn't help. Those bugs exist and I'm sharing my workarounds,
>> which are empirically determined by testing a range of files. If someone is
>> interested, I can provide private access to a repository where we have been
>> testing this. Alternatively, I could also leave those workarounds out, and
>> just skip those SEI types.
>>
>> In a previous version of this patchset, there was a concern that payload
>> data might need to be re-ordered. Meanwhile I have researched this carefully
>> and the conclusion is that this is not required.
>>
>> My detailed analysis can be found here:
>> https://gist.github.com/softworkz/36c49586a8610813a32270ee3947a932
>>
>> v4
>>
>>  * add new dependencies in makefile Now, build still works when someone uses
>>    configure --disable-decoder=h264 --disable-decoder=hevc
>>    --disable-decoder=mpegvideo --disable-decoder=mpeg1video
>>    --disable-decoder=mpeg2video --enable-libmfx
>>
>> v3
>>
>>  * frame.h: clarify doc text for av_frame_copy_side_data()
>>
>> v2
>>
>>  * qsvdec: make error handling consistent and clear
>>  * qsvdec: remove AV_CODEC_ID_MPEG1VIDEO constants
>>  * hevcdec: rename function to ff_hevc_set_side_data(), add doc text
>>
>> v3
>>
>>  * qsvdec: fix c/p error
>>
>> softworkz (6):
>>   avutil/frame: Add av_frame_copy_side_data() and
>>     av_frame_remove_all_side_data()
>>   avcodec/vpp_qsv: Copy side data from input to output frame
>>   avcodec/mpeg12dec: make mpeg_decode_user_data() accessible
>>   avcodec/hevcdec: make set_side_data() accessible
>>   avcodec/h264dec: make h264_export_frame_props() accessible
>>   avcodec/qsvdec: Implement SEI parsing for QSV decoders
>>
>>  doc/APIchanges               |   4 +
>>  libavcodec/Makefile          |   6 +-
>>  libavcodec/h264_slice.c      |  98 ++++++++-------
>>  libavcodec/h264dec.h         |   2 +
>>  libavcodec/hevcdec.c         | 117 +++++++++---------
>>  libavcodec/hevcdec.h         |   9 ++
>>  libavcodec/hevcdsp.c         |   4 +
>>  libavcodec/mpeg12.h          |  28 +++++
>>  libavcodec/mpeg12dec.c       |  40 +-----
>>  libavcodec/qsvdec.c          | 234 +++++++++++++++++++++++++++++++++++
>>  libavfilter/qsvvpp.c         |   6 +
>>  libavfilter/vf_overlay_qsv.c |  19 ++-
>>  libavutil/frame.c            |  67 ++++++----
>>  libavutil/frame.h            |  32 +++++
>>  libavutil/version.h          |   2 +-
>>  15 files changed, 494 insertions(+), 174 deletions(-)
>>
>>
>> base-commit: 6a82412bf33108111eb3f63076fd5a51349ae114
>> Published-As: 
>> https://github.com/ffstaging/FFmpeg/releases/tag/pr-ffstaging-31%2Fsoftworkz%2Fsubmit_qsv_sei-v5
>> Fetch-It-Via: git fetch https://github.com/ffstaging/FFmpeg pr-ffstaging-
>> 31/softworkz/submit_qsv_sei-v5
>> Pull-Request: https://github.com/ffstaging/FFmpeg/pull/31
>>
>> Range-diff vs v4:
>>
>>  1:  7656477360 = 1:  7656477360 avutil/frame: Add av_frame_copy_side_data()
>> and av_frame_remove_all_side_data()
>>  2:  06976606c5 = 2:  06976606c5 avcodec/vpp_qsv: Copy side data from input to
>> output frame
>>  3:  320a8a535c = 3:  320a8a535c avcodec/mpeg12dec: make
>> mpeg_decode_user_data() accessible
>>  4:  e58ad6564f = 4:  e58ad6564f avcodec/hevcdec: make set_side_data()
>> accessible
>>  5:  a57bfaebb9 = 5:  4c0b6eb4cb avcodec/h264dec: make
>> h264_export_frame_props() accessible
>>  6:  3f2588563e ! 6:  19bc00be4d avcodec/qsvdec: Implement SEI parsing for QSV
>> decoders
>>      @@ Commit message
>>       
>>           Signed-off-by: softworkz <softworkz@hotmail.com>
>>       
>>      + ## libavcodec/Makefile ##
>>      +@@ libavcodec/Makefile: OBJS-$(CONFIG_MSS34DSP)                +=
>> mss34dsp.o
>>      + OBJS-$(CONFIG_PIXBLOCKDSP)             += pixblockdsp.o
>>      + OBJS-$(CONFIG_QPELDSP)                 += qpeldsp.o
>>      + OBJS-$(CONFIG_QSV)                     += qsv.o
>>      +-OBJS-$(CONFIG_QSVDEC)                  += qsvdec.o
>>      ++OBJS-$(CONFIG_QSVDEC)                  += qsvdec.o h264_slice.o
>> h264_cabac.o h264_cavlc.o \
>>      ++                                          h264_direct.o h264_mb.o
>> h264_picture.o h264_loopfilter.o \
>>      ++                                          h264dec.o h264_refs.o cabac.o
>> hevcdec.o hevc_refs.o \
>>      ++										
>>   hevc_filter.o hevc_cabac.o hevc_mvs.o hevcpred.o hevcdsp.o \
>>      ++										
>>   h274.o dovi_rpu.o mpeg12dec.o
>>      + OBJS-$(CONFIG_QSVENC)                  += qsvenc.o
>>      + OBJS-$(CONFIG_RANGECODER)              += rangecoder.o
>>      + OBJS-$(CONFIG_RDFT)                    += rdft.o
>>      +
>>      + ## libavcodec/hevcdsp.c ##
>>      +@@
>>      +  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-
>> 1301 USA
>>      +  */
>>      + 
>>      ++#include "config_components.h"
>>      ++
>>      + #include "hevcdsp.h"
>>      + 
>>      + static const int8_t transform[32][32] = {
>>      +@@ libavcodec/hevcdsp.c: int i = 0;
>>      +         break;
>>      +     }
>>      + 
>>      ++#if CONFIG_HEVC_DECODER
>>      + #if ARCH_AARCH64
>>      +     ff_hevc_dsp_init_aarch64(hevcdsp, bit_depth);
>>      + #elif ARCH_ARM
>>      +@@ libavcodec/hevcdsp.c: int i = 0;
>>      + #elif ARCH_LOONGARCH
>>      +     ff_hevc_dsp_init_loongarch(hevcdsp, bit_depth);
>>      + #endif
>>      ++#endif
>>      + }
>>      +
>>        ## libavcodec/qsvdec.c ##
>>       @@
>>        #include "hwconfig.h"
> 
> 
> Is there any comment on this patchset ? If not, I'd like to merge it to make QSV
> decoders works with SEI info.
> 
> Thanks
> Haihao
> 

This patchset has several issues, namely:
1. It tries to share the functions that are used for processing user/SEI
data as they are, even the parts that are not intended to be used by QSV
(like the picture structure stuff for H.264 or tmpgexs in case of MPEG-1/2).
2. It tries to keep the functions where they are, leading to the
insanely long Makefile line in patch 6/6 (which I believe to be still
incomplete: mpeg12dec.o pulls in mpegvideo.o mpegvideo_dec.o (which in
turn pull in lots of dsp stuff) and where is h264dsp.o? (it seems like
there is a reliance on the H.264 parser for this)). This is the opposite
of modularity.
3. It just puts a huge Mpeg1Context in the QSVContext, although only a
miniscule part of it is actually used. One should use a small context of
its own instead.
4. It does not take into account that buffers need to be padded to be
usable by the GetBit-API.

(I have made an attempt to factor out the common parts of H.264 and
H.265 SEI handling, which should make this here much easier.)

- Andreas
Soft Works Oct. 21, 2022, 7:42 a.m. UTC | #4
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Andreas Rheinhardt
> Sent: Thursday, July 21, 2022 11:56 PM
> To: Xiang, Haihao <haihao.xiang@intel.com>; ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v5 0/6] Implement SEI parsing for
> QSV decoders
> 
> Xiang, Haihao:
> > On Fri, 2022-07-01 at 20:48 +0000, ffmpegagent wrote:
> >> Missing SEI information has always been a major drawback when
> using the QSV
> >> decoders. I used to think that there's no chance to get at the
> data without
> >> explicit implementation from the MSDK side (or doing something
> weird like
> >> parsing in parallel). It turned out that there's a hardly known
> api method
> >> that provides access to all SEI (h264/hevc) or user data
> (mpeg2video).
> >>
> >> This allows to get things like closed captions, frame packing,
> display
> >> orientation, HDR data (mastering display, content light level,
> etc.) without
> >> having to rely on those data being provided by the MSDK as
> extended buffers.
> >>
> >> The commit "Implement SEI parsing for QSV decoders" includes some
> hard-coded
> >> workarounds for MSDK bugs which I reported:
> >>
> > https://github.com/Intel-Media-
> SDK/MediaSDK/issues/2597#issuecomment-1072795311
> >>
> >> But that doesn't help. Those bugs exist and I'm sharing my
> workarounds,
> >> which are empirically determined by testing a range of files. If
> someone is
> >> interested, I can provide private access to a repository where we
> have been
> >> testing this. Alternatively, I could also leave those workarounds
> out, and
> >> just skip those SEI types.
> >>
> >> In a previous version of this patchset, there was a concern that
> payload
> >> data might need to be re-ordered. Meanwhile I have researched this
> carefully
> >> and the conclusion is that this is not required.
> >>
> >> My detailed analysis can be found here:
> >> https://gist.github.com/softworkz/36c49586a8610813a32270ee3947a932
> >>
> >> v4
> >>
> >>  * add new dependencies in makefile Now, build still works when
> someone uses
> >>    configure --disable-decoder=h264 --disable-decoder=hevc
> >>    --disable-decoder=mpegvideo --disable-decoder=mpeg1video
> >>    --disable-decoder=mpeg2video --enable-libmfx
> >>
> >> v3
> >>
> >>  * frame.h: clarify doc text for av_frame_copy_side_data()
> >>
> >> v2
> >>
> >>  * qsvdec: make error handling consistent and clear
> >>  * qsvdec: remove AV_CODEC_ID_MPEG1VIDEO constants
> >>  * hevcdec: rename function to ff_hevc_set_side_data(), add doc
> text
> >>
> >> v3
> >>
> >>  * qsvdec: fix c/p error
> >>
> >> softworkz (6):
> >>   avutil/frame: Add av_frame_copy_side_data() and
> >>     av_frame_remove_all_side_data()
> >>   avcodec/vpp_qsv: Copy side data from input to output frame
> >>   avcodec/mpeg12dec: make mpeg_decode_user_data() accessible
> >>   avcodec/hevcdec: make set_side_data() accessible
> >>   avcodec/h264dec: make h264_export_frame_props() accessible
> >>   avcodec/qsvdec: Implement SEI parsing for QSV decoders
> >>
> >>  doc/APIchanges               |   4 +
> >>  libavcodec/Makefile          |   6 +-
> >>  libavcodec/h264_slice.c      |  98 ++++++++-------
> >>  libavcodec/h264dec.h         |   2 +
> >>  libavcodec/hevcdec.c         | 117 +++++++++---------
> >>  libavcodec/hevcdec.h         |   9 ++
> >>  libavcodec/hevcdsp.c         |   4 +
> >>  libavcodec/mpeg12.h          |  28 +++++
> >>  libavcodec/mpeg12dec.c       |  40 +-----
> >>  libavcodec/qsvdec.c          | 234
> +++++++++++++++++++++++++++++++++++
> >>  libavfilter/qsvvpp.c         |   6 +
> >>  libavfilter/vf_overlay_qsv.c |  19 ++-
> >>  libavutil/frame.c            |  67 ++++++----
> >>  libavutil/frame.h            |  32 +++++
> >>  libavutil/version.h          |   2 +-
> >>  15 files changed, 494 insertions(+), 174 deletions(-)
> >>
> >>
> >> base-commit: 6a82412bf33108111eb3f63076fd5a51349ae114
> >> Published-As:
> >> https://github.com/ffstaging/FFmpeg/releases/tag/pr-ffstaging-
> 31%2Fsoftworkz%2Fsubmit_qsv_sei-v5
> >> Fetch-It-Via: git fetch https://github.com/ffstaging/FFmpeg pr-
> ffstaging-
> >> 31/softworkz/submit_qsv_sei-v5
> >> Pull-Request: https://github.com/ffstaging/FFmpeg/pull/31
> >>
> >> Range-diff vs v4:
> >>
> >>  1:  7656477360 = 1:  7656477360 avutil/frame: Add
> av_frame_copy_side_data()
> >> and av_frame_remove_all_side_data()
> >>  2:  06976606c5 = 2:  06976606c5 avcodec/vpp_qsv: Copy side data
> from input to
> >> output frame
> >>  3:  320a8a535c = 3:  320a8a535c avcodec/mpeg12dec: make
> >> mpeg_decode_user_data() accessible
> >>  4:  e58ad6564f = 4:  e58ad6564f avcodec/hevcdec: make
> set_side_data()
> >> accessible
> >>  5:  a57bfaebb9 = 5:  4c0b6eb4cb avcodec/h264dec: make
> >> h264_export_frame_props() accessible
> >>  6:  3f2588563e ! 6:  19bc00be4d avcodec/qsvdec: Implement SEI
> parsing for QSV
> >> decoders
> >>      @@ Commit message
> >>
> >>           Signed-off-by: softworkz <softworkz@hotmail.com>
> >>
> >>      + ## libavcodec/Makefile ##
> >>      +@@ libavcodec/Makefile: OBJS-$(CONFIG_MSS34DSP)
> +=
> >> mss34dsp.o
> >>      + OBJS-$(CONFIG_PIXBLOCKDSP)             += pixblockdsp.o
> >>      + OBJS-$(CONFIG_QPELDSP)                 += qpeldsp.o
> >>      + OBJS-$(CONFIG_QSV)                     += qsv.o
> >>      +-OBJS-$(CONFIG_QSVDEC)                  += qsvdec.o
> >>      ++OBJS-$(CONFIG_QSVDEC)                  += qsvdec.o
> h264_slice.o
> >> h264_cabac.o h264_cavlc.o \
> >>      ++                                          h264_direct.o
> h264_mb.o
> >> h264_picture.o h264_loopfilter.o \
> >>      ++                                          h264dec.o
> h264_refs.o cabac.o
> >> hevcdec.o hevc_refs.o \
> >>      ++
> 
> >>   hevc_filter.o hevc_cabac.o hevc_mvs.o hevcpred.o hevcdsp.o \
> >>      ++
> 
> >>   h274.o dovi_rpu.o mpeg12dec.o
> >>      + OBJS-$(CONFIG_QSVENC)                  += qsvenc.o
> >>      + OBJS-$(CONFIG_RANGECODER)              += rangecoder.o
> >>      + OBJS-$(CONFIG_RDFT)                    += rdft.o
> >>      +
> >>      + ## libavcodec/hevcdsp.c ##
> >>      +@@
> >>      +  * Foundation, Inc., 51 Franklin Street, Fifth Floor,
> Boston, MA 02110-
> >> 1301 USA
> >>      +  */
> >>      +
> >>      ++#include "config_components.h"
> >>      ++
> >>      + #include "hevcdsp.h"
> >>      +
> >>      + static const int8_t transform[32][32] = {
> >>      +@@ libavcodec/hevcdsp.c: int i = 0;
> >>      +         break;
> >>      +     }
> >>      +
> >>      ++#if CONFIG_HEVC_DECODER
> >>      + #if ARCH_AARCH64
> >>      +     ff_hevc_dsp_init_aarch64(hevcdsp, bit_depth);
> >>      + #elif ARCH_ARM
> >>      +@@ libavcodec/hevcdsp.c: int i = 0;
> >>      + #elif ARCH_LOONGARCH
> >>      +     ff_hevc_dsp_init_loongarch(hevcdsp, bit_depth);
> >>      + #endif
> >>      ++#endif
> >>      + }
> >>      +
> >>        ## libavcodec/qsvdec.c ##
> >>       @@
> >>        #include "hwconfig.h"
> >
> >
> > Is there any comment on this patchset ? If not, I'd like to merge
> it to make QSV
> > decoders works with SEI info.
> >
> > Thanks
> > Haihao
> >
> 
> This patchset has several issues, namely:
> 1. It tries to share the functions that are used for processing
> user/SEI
> data as they are, even the parts that are not intended to be used by
> QSV
> (like the picture structure stuff for H.264 or tmpgexs in case of
> MPEG-1/2).
> 2. It tries to keep the functions where they are, leading to the
> insanely long Makefile line in patch 6/6 (which I believe to be still
> incomplete: mpeg12dec.o pulls in mpegvideo.o mpegvideo_dec.o (which
> in
> turn pull in lots of dsp stuff) and where is h264dsp.o? (it seems
> like
> there is a reliance on the H.264 parser for this)). This is the
> opposite
> of modularity.
> 3. It just puts a huge Mpeg1Context in the QSVContext, although only
> a
> miniscule part of it is actually used. One should use a small context
> of
> its own instead.
> 4. It does not take into account that buffers need to be padded to be
> usable by the GetBit-API.


Hi Andreas,

thanks for pointing out (4), in fact I wasn't aware of this.

I agree to your other points (1-3). Not that I wouldn't have been 
aware of those implications, I've just been afraid that larger refactorings
could have minimized acceptance.


> (I have made an attempt to factor out the common parts of H.264 and
> H.265 SEI handling, which should make this here much easier.)

Your patchset would in fact be very helpful and allow me to provide 
a much better and focused revision.
Though, it is still pending at this time - are you planning to push it?


Thanks,
softworkz