> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of > Andreas Rheinhardt > Sent: Thursday, July 21, 2022 11:56 PM > To: Xiang, Haihao <haihao.xi...@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 <softwo...@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
