[libav-devel] [PATCH] arm: vp9lpf: Fix a typo in a comment about the register layout
--- libavcodec/aarch64/vp9lpf_neon.S | 2 +- libavcodec/arm/vp9lpf_neon.S | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/libavcodec/aarch64/vp9lpf_neon.S b/libavcodec/aarch64/vp9lpf_neon.S index e9c497096b..f68b54a2ee 100644 --- a/libavcodec/aarch64/vp9lpf_neon.S +++ b/libavcodec/aarch64/vp9lpf_neon.S @@ -415,7 +415,7 @@ 1: // flat8out -// This writes all outputs into v2-v17 (skipping v6 and v16). +// This writes all outputs into v2-v17 (skipping v7 and v16). // If this part is skipped, the output is read from v21-v26 (which is the input // to this section). ushll_szv0.8h, v1.8h, v16, #3, \sz // 8 * v16 diff --git a/libavcodec/arm/vp9lpf_neon.S b/libavcodec/arm/vp9lpf_neon.S index ae782b2ed0..e30f0cd5b4 100644 --- a/libavcodec/arm/vp9lpf_neon.S +++ b/libavcodec/arm/vp9lpf_neon.S @@ -362,7 +362,7 @@ beq 8f @ flat8out -@ This writes all outputs into d2-d17 (skipping d6 and d16). +@ This writes all outputs into d2-d17 (skipping d7 and d16). @ If this part is skipped, the output is read from d21-d26 (which is the input @ to this section). vshll.u8q0, d16, #3 @ 8 * d16 -- 2.20.1 (Apple Git-117) ___ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel
Re: [libav-devel] [PATCH 1/2] configure: Try adding -D_POSIX_C_SOURCE=200112 -D_XOPEN_SOURCE=600 for mingw as well
On Tue, 16 Apr 2019, Martin Storsjö wrote:
On Tue, 16 Apr 2019, Diego Biurrun wrote:
On Sun, Apr 14, 2019 at 09:33:40PM +0300, Martin Storsjö wrote:
On Sun, 14 Apr 2019, Diego Biurrun wrote:
> On Sat, Apr 13, 2019 at 12:58:40AM +0300, Martin Storsjö wrote:
> > On Fri, 12 Apr 2019, Luca Barbato wrote:
> > > On 11/04/2019 15:35, Martin Storsjö wrote:
> > > > On Wed, 10 Apr 2019, Luca Barbato wrote:
> > > > > On 10/04/2019 10:48, Martin Storsjö wrote:
> > > > > > Mingw headers have got header inline implementations of
localtime_r
> > > > > > and gmtime_r, but only visible if certain posix thread safe
functions
> > > > > > have been requested.
> > > > > > > > > > this is a preparatory step for improving the detection
of those
> > > > > > functions.
> > > > > > ---
> > > > > > An alternative fix is also provided in a different patch
series,
> > > > > > by adjusting libavutil/time_internal.h.
> > > > > > > > Seems fine to me.
> > > > > > Which ones do you mean - this series of 2 patches, the other
one, or both?
> > > > > > This series seems fine to me.
> >
> > Ok. FWIW, the change in mingw-w64 that broke it was reverted (there
was a
> > similar issue within gcc as well), but I guess this change probably is
good
> > to make anyway.
>
> I generally don't think that adding workarounds for foreign bugs is a
> sustainable strategy,
Well, the idea of prefixing local system function fallbacks/replacements
isn't so much of a "workaround" as a sensible idea in general IMO. This is
a
pattern that already is used e.g. for ff_getaddrinfo, ff_poll etc.
That is, regardless of what the reason for using a fallback is (the real
function does not exist, the real function is declared in headers but
missing in libs, the real function exists but we want to avoid it because
it's buggy, etc), the pattern of
#include
static inline ff_systemfunc() {
...
}
#define systemfunc ff_systemfunc
should always be safe. So I think that should be a generally beneficial
change in any case as well.
IIRC we only do that within libavformat and use a different pattern within
libavutil. Then again, my code knowledge might be getting a bit rusty.
True, e.g. libavutil/libm.h does define some static inline functions
unprefixed as well.
Nevertheless, using prefixes for fallback functions is not a
workaround/hack in my book, but a sane and healthy development practice.
> but I clearly prefer the configure change.
Well, the check_func_headers change obviously is for the better, yes.
Adding
the _POSIX_C_SOURCE define when building for mingw most probably also is
sensible, but the fact that we add it manually to most OSes, while we
don't
add it automatically for all, makes it a little less clear cut.
Switching from trying to set some flags globally for all platforms,
inevitably
hitting a snag on some fringe system, then adding an exception for that
system,
to setting flags by platform and strictly only when necessary on that
platform,
is - oddly enough - one of the single biggest improvements to the whole
configure machinery.
Sure, I generally agree with that. I was generally a bit weary of forcing
the posix defines on other systems, but I generally think it should be
good for this case, as it reduces inconsistencies between
available/visible functions.
So I'm very weary of changes in that area due to having been burned so
often
in the past. If the change was motivated by a bug (since fixed) in mingw,
then we should not add workarounds for it.
Well it's not quite as simple. The immediate issue is gone again, but the
general underlying issue remains.
The TL;DR version is:
- mingw-w64 contains localtime_r/gmtime_r, but only visible if posix
thread safe functions have been requested by some means. We currently
don't detect these in configure. In practice, the posix thread safe
functions define could be enabled transitively by some other included
header (which has also been somewhat mitigated within mingw-w64). To
safeguard against this inconsistency, defining it in configure would be
helpful IMO.
- Even if localtime_r was visible from mingw-w64 headers, it used to not
conflict with ours, because the mingw-w64 was defined as extern inline,
while ours was static inline. The mingw-w64 headers were changed to define
this as static inline, and later reverted again.
Anyway, I've presented my arguments. I trust you to make a good decision.
Push at your discretion.
Well in that case, I'd push all four paches.
Pushed all four - thanks for the discussion!
// Martin
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
Re: [libav-devel] [PATCH 1/2] configure: Try adding -D_POSIX_C_SOURCE=200112 -D_XOPEN_SOURCE=600 for mingw as well
On Tue, 16 Apr 2019, Diego Biurrun wrote:
On Sun, Apr 14, 2019 at 09:33:40PM +0300, Martin Storsjö wrote:
On Sun, 14 Apr 2019, Diego Biurrun wrote:
> On Sat, Apr 13, 2019 at 12:58:40AM +0300, Martin Storsjö wrote:
> > On Fri, 12 Apr 2019, Luca Barbato wrote:
> > > On 11/04/2019 15:35, Martin Storsjö wrote:
> > > > On Wed, 10 Apr 2019, Luca Barbato wrote:
> > > > > On 10/04/2019 10:48, Martin Storsjö wrote:
> > > > > > Mingw headers have got header inline implementations of localtime_r
> > > > > > and gmtime_r, but only visible if certain posix thread safe
functions
> > > > > > have been requested.
> > > > > > > > > > this is a preparatory step for improving the detection of
those
> > > > > > functions.
> > > > > > ---
> > > > > > An alternative fix is also provided in a different patch series,
> > > > > > by adjusting libavutil/time_internal.h.
> > > > > > > > Seems fine to me.
> > > > > > Which ones do you mean - this series of 2 patches, the other one,
or both?
> > > > > > This series seems fine to me.
> >
> > Ok. FWIW, the change in mingw-w64 that broke it was reverted (there was a
> > similar issue within gcc as well), but I guess this change probably is good
> > to make anyway.
>
> I generally don't think that adding workarounds for foreign bugs is a
> sustainable strategy,
Well, the idea of prefixing local system function fallbacks/replacements
isn't so much of a "workaround" as a sensible idea in general IMO. This is a
pattern that already is used e.g. for ff_getaddrinfo, ff_poll etc.
That is, regardless of what the reason for using a fallback is (the real
function does not exist, the real function is declared in headers but
missing in libs, the real function exists but we want to avoid it because
it's buggy, etc), the pattern of
#include
static inline ff_systemfunc() {
...
}
#define systemfunc ff_systemfunc
should always be safe. So I think that should be a generally beneficial
change in any case as well.
IIRC we only do that within libavformat and use a different pattern within
libavutil. Then again, my code knowledge might be getting a bit rusty.
True, e.g. libavutil/libm.h does define some static inline functions
unprefixed as well.
Nevertheless, using prefixes for fallback functions is not a
workaround/hack in my book, but a sane and healthy development practice.
> but I clearly prefer the configure change.
Well, the check_func_headers change obviously is for the better, yes. Adding
the _POSIX_C_SOURCE define when building for mingw most probably also is
sensible, but the fact that we add it manually to most OSes, while we don't
add it automatically for all, makes it a little less clear cut.
Switching from trying to set some flags globally for all platforms, inevitably
hitting a snag on some fringe system, then adding an exception for that system,
to setting flags by platform and strictly only when necessary on that platform,
is - oddly enough - one of the single biggest improvements to the whole
configure machinery.
Sure, I generally agree with that. I was generally a bit weary of forcing
the posix defines on other systems, but I generally think it should be
good for this case, as it reduces inconsistencies between
available/visible functions.
So I'm very weary of changes in that area due to having been burned so often
in the past. If the change was motivated by a bug (since fixed) in mingw,
then we should not add workarounds for it.
Well it's not quite as simple. The immediate issue is gone again, but the
general underlying issue remains.
The TL;DR version is:
- mingw-w64 contains localtime_r/gmtime_r, but only visible if posix
thread safe functions have been requested by some means. We currently
don't detect these in configure. In practice, the posix thread safe
functions define could be enabled transitively by some other included
header (which has also been somewhat mitigated within mingw-w64). To
safeguard against this inconsistency, defining it in configure would be
helpful IMO.
- Even if localtime_r was visible from mingw-w64 headers, it used to not
conflict with ours, because the mingw-w64 was defined as extern inline,
while ours was static inline. The mingw-w64 headers were changed to define
this as static inline, and later reverted again.
Anyway, I've presented my arguments. I trust you to make a good decision.
Push at your discretion.
Well in that case, I'd push all four paches.
// Martin
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
Re: [libav-devel] [PATCH 1/2] configure: Try adding -D_POSIX_C_SOURCE=200112 -D_XOPEN_SOURCE=600 for mingw as well
On Sun, Apr 14, 2019 at 09:33:40PM +0300, Martin Storsjö wrote:
> On Sun, 14 Apr 2019, Diego Biurrun wrote:
> > On Sat, Apr 13, 2019 at 12:58:40AM +0300, Martin Storsjö wrote:
> > > On Fri, 12 Apr 2019, Luca Barbato wrote:
> > > > On 11/04/2019 15:35, Martin Storsjö wrote:
> > > > > On Wed, 10 Apr 2019, Luca Barbato wrote:
> > > > > > On 10/04/2019 10:48, Martin Storsjö wrote:
> > > > > > > Mingw headers have got header inline implementations of
> > > > > > > localtime_r
> > > > > > > and gmtime_r, but only visible if certain posix thread safe
> > > > > > > functions
> > > > > > > have been requested.
> > > > > > > > > > > this is a preparatory step for improving the detection of
> > > > > > > > > > > those
> > > > > > > functions.
> > > > > > > ---
> > > > > > > An alternative fix is also provided in a different patch series,
> > > > > > > by adjusting libavutil/time_internal.h.
> > > > > > > > > Seems fine to me.
> > > > > > > Which ones do you mean - this series of 2 patches, the other one,
> > > > > > > or both?
> > > > > > > This series seems fine to me.
> > >
> > > Ok. FWIW, the change in mingw-w64 that broke it was reverted (there was a
> > > similar issue within gcc as well), but I guess this change probably is
> > > good
> > > to make anyway.
> >
> > I generally don't think that adding workarounds for foreign bugs is a
> > sustainable strategy,
>
> Well, the idea of prefixing local system function fallbacks/replacements
> isn't so much of a "workaround" as a sensible idea in general IMO. This is a
> pattern that already is used e.g. for ff_getaddrinfo, ff_poll etc.
>
> That is, regardless of what the reason for using a fallback is (the real
> function does not exist, the real function is declared in headers but
> missing in libs, the real function exists but we want to avoid it because
> it's buggy, etc), the pattern of
>
> #include
> static inline ff_systemfunc() {
> ...
> }
> #define systemfunc ff_systemfunc
>
> should always be safe. So I think that should be a generally beneficial
> change in any case as well.
IIRC we only do that within libavformat and use a different pattern within
libavutil. Then again, my code knowledge might be getting a bit rusty.
> > but I clearly prefer the configure change.
>
> Well, the check_func_headers change obviously is for the better, yes. Adding
> the _POSIX_C_SOURCE define when building for mingw most probably also is
> sensible, but the fact that we add it manually to most OSes, while we don't
> add it automatically for all, makes it a little less clear cut.
Switching from trying to set some flags globally for all platforms, inevitably
hitting a snag on some fringe system, then adding an exception for that system,
to setting flags by platform and strictly only when necessary on that platform,
is - oddly enough - one of the single biggest improvements to the whole
configure machinery.
So I'm very weary of changes in that area due to having been burned so often
in the past. If the change was motivated by a bug (since fixed) in mingw,
then we should not add workarounds for it.
Anyway, I've presented my arguments. I trust you to make a good decision.
Push at your discretion.
Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
