diff mbox series

[FFmpeg-devel] libaom: Dont use aom_codec_av1_dx_algo.

Message ID CAHVN4mj9ha4iq2OPMdF5tyiWP2uToZ1TvRnZ1AJEypgDpgZNBA@mail.gmail.com
State New
Headers show
Series [FFmpeg-devel] libaom: Dont use aom_codec_av1_dx_algo.
Related show

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Matt Oliver Oct. 20, 2021, 4:52 p.m. UTC
This fixes linking errors where variables cannot be correctly linked in
from an external shared library such as with msvc (requires dllimport which
is not used by libaom). Instead just call the function that returns the
same variable.
---
 libavcodec/libaomdec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

James Zern Oct. 20, 2021, 5:49 p.m. UTC | #1
On Wed, Oct 20, 2021 at 9:59 AM Matt Oliver <protogonoi@gmail.com> wrote:
>
> This fixes linking errors where variables cannot be correctly linked in
> from an external shared library such as with msvc (requires dllimport which
> is not used by libaom). Instead just call the function that returns the
> same variable.
> ---
>  libavcodec/libaomdec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavcodec/libaomdec.c b/libavcodec/libaomdec.c
> index 75ecc08970..d6b822fda2 100644
> --- a/libavcodec/libaomdec.c
> +++ b/libavcodec/libaomdec.c
> @@ -241,7 +241,7 @@ static av_cold int aom_free(AVCodecContext *avctx)
>
>  static av_cold int av1_init(AVCodecContext *avctx)
>  {
> -    return aom_init(avctx, &aom_codec_av1_dx_algo);
> +    return aom_init(avctx, aom_codec_av1_dx());
>  }
>

lgtm.
Matt Oliver Oct. 30, 2021, 2:43 a.m. UTC | #2
>
> lgtm.
>

Thanks,
I will push this assuming no other comments.
James Almer Oct. 30, 2021, 3:02 a.m. UTC | #3
On 10/29/2021 11:43 PM, Matt Oliver wrote:
>>
>> lgtm.
>>
> 
> Thanks,
> I will push this assuming no other comments.

Does this compile and link with libaom 1.0.0? We unfortunately still 
support that version (despite being unusable speed wise) because distros 
like Debian still ship it.
Matt Oliver Oct. 30, 2021, 3:13 a.m. UTC | #4
>
> Does this compile and link with libaom 1.0.0? We unfortunately still
> support that version (despite being unusable speed wise) because distros
> like Debian still ship it.
>

Just looked it up and yes version 1.0.0 includes the required
aom_codec_av1_dx() function (line 37 of aomdx.h for those interested).
Matt Oliver Oct. 30, 2021, 3:50 a.m. UTC | #5
On Sat, 30 Oct 2021 at 14:13, Matt Oliver <protogonoi@gmail.com> wrote:

> Does this compile and link with libaom 1.0.0? We unfortunately still
>> support that version (despite being unusable speed wise) because distros
>> like Debian still ship it.
>>
>
> Just looked it up and yes version 1.0.0 includes the required
> aom_codec_av1_dx() function (line 37 of aomdx.h for those interested).
>

I also noticed that libvpx does the same thing as libaom where it uses the
direct variable instead of the function returning the variable. libvpx
doesnt currently support static compilation so it's not an issue that has
come up but was wondering whether it was worth submitting a patch
preemptively fixing that issue as well.
James Zern Nov. 1, 2021, 5:39 p.m. UTC | #6
On Fri, Oct 29, 2021 at 8:13 PM Matt Oliver <protogonoi@gmail.com> wrote:
>
> >
> > Does this compile and link with libaom 1.0.0? We unfortunately still
> > support that version (despite being unusable speed wise) because distros
> > like Debian still ship it.
> >
>
> Just looked it up and yes version 1.0.0 includes the required
> aom_codec_av1_dx() function (line 37 of aomdx.h for those interested).
>

There's some coverage of supported versions of libvpx and libaom to
help catch compatibility issues here:
https://build.webmproject.org/jenkins/view/libvpx%20nightly/job/libvpx__ffmpeg/
James Zern Nov. 1, 2021, 5:40 p.m. UTC | #7
On Fri, Oct 29, 2021 at 8:51 PM Matt Oliver <protogonoi@gmail.com> wrote:
>
> On Sat, 30 Oct 2021 at 14:13, Matt Oliver <protogonoi@gmail.com> wrote:
>
> > Does this compile and link with libaom 1.0.0? We unfortunately still
> >> support that version (despite being unusable speed wise) because distros
> >> like Debian still ship it.
> >>
> >
> > Just looked it up and yes version 1.0.0 includes the required
> > aom_codec_av1_dx() function (line 37 of aomdx.h for those interested).
> >
>
> I also noticed that libvpx does the same thing as libaom where it uses the
> direct variable instead of the function returning the variable. libvpx
> doesnt currently support static compilation so it's not an issue that has
> come up but was wondering whether it was worth submitting a patch
> preemptively fixing that issue as well.
>

That would be fine. By static compilation do you mean generating an
import lib for this scenario?
Matt Oliver Nov. 2, 2021, 1:11 a.m. UTC | #8
On Tue, 2 Nov 2021 at 04:41, James Zern <jzern-at-google.com@ffmpeg.org>
wrote:

> On Fri, Oct 29, 2021 at 8:51 PM Matt Oliver <protogonoi@gmail.com> wrote:
> >
> > On Sat, 30 Oct 2021 at 14:13, Matt Oliver <protogonoi@gmail.com> wrote:
> >
> > > Does this compile and link with libaom 1.0.0? We unfortunately still
> > >> support that version (despite being unusable speed wise) because
> distros
> > >> like Debian still ship it.
> > >>
> > >
> > > Just looked it up and yes version 1.0.0 includes the required
> > > aom_codec_av1_dx() function (line 37 of aomdx.h for those interested).
> > >
> >
> > I also noticed that libvpx does the same thing as libaom where it uses
> the
> > direct variable instead of the function returning the variable. libvpx
> > doesnt currently support static compilation so it's not an issue that has
> > come up but was wondering whether it was worth submitting a patch
> > preemptively fixing that issue as well.
> >
>
> That would be fine. By static compilation do you mean generating an
> import lib for this scenario?
>

That was actually a typo, I meant libvpx doesnt support 'shared'
compilation on windows (the configure script only allows it to be specified
on ELF platforms). I'm not familiar with why libvpx doesnt currently allow
it but if it did then ffmpeg would have the same issues as with libaom that
was fixed with this patch. Which is why I was wondering if people were fine
with me preemptively fixing it by patching ffmpeg now instead of waiting
for it to be a potential issue down the line.
James Zern Nov. 2, 2021, 7:23 p.m. UTC | #9
On Mon, Nov 1, 2021 at 6:11 PM Matt Oliver <protogonoi@gmail.com> wrote:
>
> On Tue, 2 Nov 2021 at 04:41, James Zern <jzern-at-google.com@ffmpeg.org>
> wrote:
>
> > On Fri, Oct 29, 2021 at 8:51 PM Matt Oliver <protogonoi@gmail.com> wrote:
> > >
> > > On Sat, 30 Oct 2021 at 14:13, Matt Oliver <protogonoi@gmail.com> wrote:
> > >
> > > > Does this compile and link with libaom 1.0.0? We unfortunately still
> > > >> support that version (despite being unusable speed wise) because
> > distros
> > > >> like Debian still ship it.
> > > >>
> > > >
> > > > Just looked it up and yes version 1.0.0 includes the required
> > > > aom_codec_av1_dx() function (line 37 of aomdx.h for those interested).
> > > >
> > >
> > > I also noticed that libvpx does the same thing as libaom where it uses
> > the
> > > direct variable instead of the function returning the variable. libvpx
> > > doesnt currently support static compilation so it's not an issue that has
> > > come up but was wondering whether it was worth submitting a patch
> > > preemptively fixing that issue as well.
> > >
> >
> > That would be fine. By static compilation do you mean generating an
> > import lib for this scenario?
> >
>
> That was actually a typo, I meant libvpx doesnt support 'shared'
> compilation on windows (the configure script only allows it to be specified
> on ELF platforms). I'm not familiar with why libvpx doesnt currently allow
> it but if it did then ffmpeg would have the same issues as with libaom that
> was fixed with this patch.

There's no reason someone couldn't build libvpx this way, but
configure never had support added for it; libaom picked up support
through its transition to cmake.

> Which is why I was wondering if people were fine
> with me preemptively fixing it by patching ffmpeg now instead of waiting
> for it to be a potential issue down the line.
>

That's fine by me.
diff mbox series

Patch

diff --git a/libavcodec/libaomdec.c b/libavcodec/libaomdec.c
index 75ecc08970..d6b822fda2 100644
--- a/libavcodec/libaomdec.c
+++ b/libavcodec/libaomdec.c
@@ -241,7 +241,7 @@  static av_cold int aom_free(AVCodecContext *avctx)

 static av_cold int av1_init(AVCodecContext *avctx)
 {
-    return aom_init(avctx, &aom_codec_av1_dx_algo);
+    return aom_init(avctx, aom_codec_av1_dx());
 }

 const AVCodec ff_libaom_av1_decoder = {