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

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

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