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

[Xiang, Haihao]

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.443304-2-haihao.xiang@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.

Thanks
Haihao