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

[Soft Works <softworkz@hotmail.com>]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Xiang, Haihao
> Sent: Sunday, May 1, 2022 6:52 AM
> To: anton@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-bounces@ffmpeg.org> On Behalf Of
> > > Xiang, Haihao
> > > Sent: Sunday, May 1, 2022 4:16 AM
> > > To: anton@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-bounces@ffmpeg.org> On Behalf
> Of
> > > > > Xiang, Haihao
> > > > > Sent: Wednesday, April 6, 2022 5:38 AM
> > > > > To: anton@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.xiang@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@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@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).

Best,
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-request@ffmpeg.org with subject "unsubscribe".