[FFmpeg-devel] [PATCH v7 05/10] qsv: build audio related code when MFX_VERSION < 2.0

[Xiang, Haihao]

On Sun, 2022-05-01 at 05:13 +0000, Soft Works wrote:
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> > Xiang, Haihao
> > Sent: Sunday, May 1, 2022 6:52 AM
> > To: anton at khirnov.net; ffmpeg-devel at 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-bounces at ffmpeg.org> On Behalf Of
> > > > Xiang, Haihao
> > > > Sent: Sunday, May 1, 2022 4:16 AM
> > > > To: anton at khirnov.net; ffmpeg-devel at 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-bounces at ffmpeg.org> On Behalf
> > 
> > Of
> > > > > > Xiang, Haihao
> > > > > > Sent: Wednesday, April 6, 2022 5:38 AM
> > > > > > To: anton at khirnov.net; ffmpeg-devel at 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.xiang at 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.xiang at 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.xiang at 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