On Sun, 2022-05-01 at 05:13 +0000, Soft Works wrote: > > -----Original Message----- > > From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of > > Xiang, Haihao > > Sent: Sunday, May 1, 2022 6:52 AM > > To: an...@khirnov.net; ffmpeg-devel@ffmpeg.org > > Subject: Re: [FFmpeg-devel] [PATCH v7 05/10] qsv: build audio related > > code when MFX_VERSION < 2.0 > > > > On Sun, 2022-05-01 at 03:10 +0000, Soft Works wrote: > > > > -----Original Message----- > > > > From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of > > > > Xiang, Haihao > > > > Sent: Sunday, May 1, 2022 4:16 AM > > > > To: an...@khirnov.net; ffmpeg-devel@ffmpeg.org > > > > Subject: Re: [FFmpeg-devel] [PATCH v7 05/10] qsv: build audio > > > > related > > > > code when MFX_VERSION < 2.0 > > > > > > > > On Sat, 2022-04-30 at 16:51 +0000, Soft Works wrote: > > > > > > -----Original Message----- > > > > > > From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf > > > > Of > > > > > > Xiang, Haihao > > > > > > Sent: Wednesday, April 6, 2022 5:38 AM > > > > > > To: an...@khirnov.net; ffmpeg-devel@ffmpeg.org > > > > > > Subject: Re: [FFmpeg-devel] [PATCH v7 05/10] qsv: build audio > > > > > > > > related > > > > > > code when MFX_VERSION < 2.0 > > > > > > > > > > > > On Tue, 2022-04-05 at 13:50 +0200, Anton Khirnov wrote: > > > > > > > Quoting Xiang, Haihao (2022-03-11 09:16:25) > > > > > > > > From: Haihao Xiang <haihao.xi...@intel.com> > > > > > > > > > > > > > > > > Audio isn't supported for MFX_VERSION >= 2.0[1][2]. This > > > > is in > > > > > > > > preparation for oneVPL support > > > > > > > > > > > > > > > > [1]: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://spec.oneapi.io/versions/latest/elements/oneVPL/source/VPL_inte > > > > > > l_media_sdk.html#msdk-full-name-feature-removals > > > > > > > > [2]: https://github.com/oneapi-src/oneVPL > > > > > > > > --- > > > > > > > > libavcodec/qsv.c | 5 +++++ > > > > > > > > libavfilter/qsvvpp.c | 6 ++++++ > > > > > > > > libavfilter/qsvvpp.h | 2 ++ > > > > > > > > 3 files changed, 13 insertions(+) > > > > > > > > > > > > > > Why not just remove this completely? > > > > > > > None of our QSV code does anything with audio. > > > > > > > > > > > > It was removed in an older version, however someone objected > > > > the > > > > > > removal of > > > > > > this. See > > > > > > > > > > > > > > > > > > https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200819061023.44330 > > > > > > 4-2-haihao.xi...@intel.com/ > > > > > > > > > > I think this was a misunderstanding. I see not objection. One > > > > was > > > > > just asking "why" and the other one had missed the point that > > > > audio > > > > > has never been functional. > > > > > > > > Please find the comment below in the original thread > > > > > > > > "This seems like a generic translation from the library errors to > > > > FF > > > > error > > > > codes. So even if we'll never touch the audio functionality of it, > > > > I'd > > > > prefer > > > > to have that struct complete already" > > > > > > > > So my understanding was that the reviewer preferred to keep the > > > > audio > > > > stuff > > > > unchanged for libmfx. > > > > > > Hm, I hadn't see that here: > > > > > > > > > > https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200819061023.44330 > > 4-2-haihao.xi...@intel.com/ > > > > > > Considering the text again: > > > > > > > "This seems like a generic translation from the library errors to > > > > FF > > > > error > > > > codes. So even if we'll never touch the audio functionality of it, > > > > I'd > > > > prefer > > > > to have that struct complete already" > > > > > > I understand the idea. Normally those lines wouldn't hurt. But now, > > > > that > > > we're facing some kind of "#ifdef hell" anyway, I think it would be > > > > much > > > better to minimize this as much as possible, and there's really no > > > > point > > > in translating audio error codes. > > > Also, the struct has never been really complete. Instead of > > > > retaining > > > unused audio error codes, we should better add those that are > > > > missing > > > (like -21, -22 and others) and relevant. > > > > > > @Thilo - can we get you warm with that? > > > > > > As an alternative, we could simply replace the two audio definitions > > > > with > > > plain integer values (MFX_ERR_INVALID_AUDIO_PARAM >> -19) > > > > > > > > > This is a small bit only, but before adding the oneVPL stuff, I > > > > think > > > we should consolidate the conditional stuff as much as possible. > > > > > > As discussed before, we also need to settle for a minimum libmfx SDK > > > version (compile-time, not runtime!). This will allow to drop quite > > > an amount of conditional code, and this cleanup should be done > > > > before > > > getting to oneVPL. > > > > Sure, I'll submit a patch for it. > > > > > > > > Another thing that is a bit unfortunate is that we are duplicating > > > > this > > > error mapping struct in qsv.c and qsvvpp.c. > > > I don't mean that it should be linked as an external between > > > > avfilter > > > and avcodec, but it should come (be included) from a single file. > > > > We moved the static error table to a .h in the past however it > > resulted in link > > error when building FFmpeg with static libraries. > > That's why I said that it doesn't need to be exported - just a > single file that can be included/inlined in both cases where it > is used (lavcodec, lavfilter).
Thanks for your comment, I'll consider to change it later. BTW we don't get response from Thilo, I'll keep this patch unchanged in the new version. I think we may update it in the future if there are more comments. Thanks Haihao _______________________________________________ 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".
