Re: [FFmpeg-devel] [PATCH 5/5] aarch64: me_cmp: Don't do uaddlv once per iteration
On Sat, 16 Jul 2022, Michael Niedermayer wrote:
On Sat, Jul 16, 2022 at 03:30:23PM +0300, Martin Storsjö wrote:
On Sat, 16 Jul 2022, Michael Niedermayer wrote:
On Sat, Jul 16, 2022 at 12:25:37AM +0300, Martin Storsjö wrote:
On Fri, 15 Jul 2022, Michael Niedermayer wrote:
On Fri, Jul 15, 2022 at 10:56:03PM +0300, Martin Storsjö wrote:
On Fri, 15 Jul 2022, Swinney, Jonathan wrote:
If the max height is just 16, then this should be fine. I assumed that h
could have a much higher value (>1024), but if that is not the case,
then this is a useful optimization.
At least according to the me_cmp.h header, which says:
/* Motion estimation:
* h is limited to { width / 2, width, 2 * width },
* but never larger than 16 and never smaller than 2.
* Although currently h < 4 is not used as functions with
* width < 8 are neither used nor implemented. */
These rules where written with support for encoding of all
standard formats in mind at the time that was written.
today it may make sense to extend these rules to cover the
things which where created since then
Right, but if that suddenly changes, such a change also must expect that it
might need updates to all assembly implementations that implement that
interface currently. Right now, both the defacto case (any callers in the
codebase) and the explicit documentation says that it can't be called with
parameters outside of that range.
What i meant was that newly added functions should be more flexible than
these old rules. That is 2 sets of rules
1. What a caller ATM can do and expect to work (thats whats written there)
2. What an implementor of new functions should make sure is supported
With 2., do you mean if adding a new function into the same struct, or if
implementing the existing pix_abs[0][..] functions?
i would say both
If you mean new implementations of the existing function interface, you say
they "should be more flexible". How flexible must they be? Is it ok to
assume h<=256 for the w=16 functions?
i think thats fine
Ok, I'll go ahead and push this then.
Gradually increasing the requirements for existing function interfaces like
you suggest is really problematic.
why ?
iam really just saying
"when you add new code, dont base it on old limitations"
For this case, I just quoted what the header said, which seemed
authoritative to me - but it's fine for me with a wider spec too, up to
h=256. But saying arbitrarily "any height" really inhibits what you can do
in asm.
Also if I shouldn't reference that limitation in the header, please
update/reword it, as it's actively misleading for anyone working on
this right now.
And in general, we can design for an intended use case and calculate
whether it should work or not - but as long as it's not tested, those
cases will often have hidden bugs - see e.g. patch 1/5 in this series.
// Martin
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 5/5] aarch64: me_cmp: Don't do uaddlv once per iteration
On Sat, Jul 16, 2022 at 03:30:23PM +0300, Martin Storsjö wrote:
> On Sat, 16 Jul 2022, Michael Niedermayer wrote:
>
> > On Sat, Jul 16, 2022 at 12:25:37AM +0300, Martin Storsjö wrote:
> > > On Fri, 15 Jul 2022, Michael Niedermayer wrote:
> > >
> > > > On Fri, Jul 15, 2022 at 10:56:03PM +0300, Martin Storsjö wrote:
> > > > > On Fri, 15 Jul 2022, Swinney, Jonathan wrote:
> > > > >
> > > > > > If the max height is just 16, then this should be fine. I assumed
> > > > > > that h
> > > > > > could have a much higher value (>1024), but if that is not the case,
> > > > > > then this is a useful optimization.
> > > > >
> > > > > At least according to the me_cmp.h header, which says:
> > > > >
> > > > > /* Motion estimation:
> > > > > * h is limited to { width / 2, width, 2 * width },
> > > > > * but never larger than 16 and never smaller than 2.
> > > > > * Although currently h < 4 is not used as functions with
> > > > > * width < 8 are neither used nor implemented. */
> > > >
> > > > These rules where written with support for encoding of all
> > > > standard formats in mind at the time that was written.
> > > > today it may make sense to extend these rules to cover the
> > > > things which where created since then
> > >
> > > Right, but if that suddenly changes, such a change also must expect that
> > > it
> > > might need updates to all assembly implementations that implement that
> > > interface currently. Right now, both the defacto case (any callers in the
> > > codebase) and the explicit documentation says that it can't be called with
> > > parameters outside of that range.
> >
> > What i meant was that newly added functions should be more flexible than
> > these old rules. That is 2 sets of rules
> > 1. What a caller ATM can do and expect to work (thats whats written there)
> > 2. What an implementor of new functions should make sure is supported
>
> With 2., do you mean if adding a new function into the same struct, or if
> implementing the existing pix_abs[0][..] functions?
i would say both
>
> If you mean new implementations of the existing function interface, you say
> they "should be more flexible". How flexible must they be? Is it ok to
> assume h<=256 for the w=16 functions?
i think thats fine
>
> Gradually increasing the requirements for existing function interfaces like
> you suggest is really problematic.
why ?
iam really just saying
"when you add new code, dont base it on old limitations"
And i suggest this because its much easier to do when a function is added than
when later one wants to use it
When i originally wrote this list of restrictions it didnt list what our
code actually used but tried to look ahead to what we would need when we
write encoders for all standard formats at that time. We never wrote a
h264 encoder but this API was designed to support it.
>
> If we want to require more of the functions, we should document it, and
> extend the checkasm test to test that new requirement - which also extends
> the requirement to the existing functions. If we don't have a checkasm test
> for the required behaviour, we can pretty much assume it's broken, even in
> new implementations.
yes
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
No snowflake in an avalanche ever feels responsible. -- Voltaire
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 5/5] aarch64: me_cmp: Don't do uaddlv once per iteration
On Sat, Jul 16, 2022 at 08:50:24PM +0800, Ronald S. Bultje wrote: > Hi, > > On Sat, Jul 16, 2022 at 7:23 PM Michael Niedermayer > wrote: > > > What i meant was that newly added functions should be more flexible than > > these old rules. That is 2 sets of rules > > 1. What a caller ATM can do and expect to work (thats whats written there) > > 2. What an implementor of new functions should make sure is supported > > > > What's the use case exactly that you'd like to support that we currently > don't? The idea is to have general purpose functions which can be used if someone wants to write a new encoder. (this is kind of not a different goal, its rather that the original limitations where based on mpeg2/mpeg4/h264 which was the most flexible at the time) thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Take away the freedom of one citizen and you will be jailed, take away the freedom of all citizens and you will be congratulated by your peers in Parliament. signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 5/5] aarch64: me_cmp: Don't do uaddlv once per iteration
Hi, On Sat, Jul 16, 2022 at 7:23 PM Michael Niedermayer wrote: > What i meant was that newly added functions should be more flexible than > these old rules. That is 2 sets of rules > 1. What a caller ATM can do and expect to work (thats whats written there) > 2. What an implementor of new functions should make sure is supported > What's the use case exactly that you'd like to support that we currently don't? Ronald ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 5/5] aarch64: me_cmp: Don't do uaddlv once per iteration
On Sat, 16 Jul 2022, Michael Niedermayer wrote:
On Sat, Jul 16, 2022 at 12:25:37AM +0300, Martin Storsjö wrote:
On Fri, 15 Jul 2022, Michael Niedermayer wrote:
On Fri, Jul 15, 2022 at 10:56:03PM +0300, Martin Storsjö wrote:
On Fri, 15 Jul 2022, Swinney, Jonathan wrote:
If the max height is just 16, then this should be fine. I assumed that h
could have a much higher value (>1024), but if that is not the case,
then this is a useful optimization.
At least according to the me_cmp.h header, which says:
/* Motion estimation:
* h is limited to { width / 2, width, 2 * width },
* but never larger than 16 and never smaller than 2.
* Although currently h < 4 is not used as functions with
* width < 8 are neither used nor implemented. */
These rules where written with support for encoding of all
standard formats in mind at the time that was written.
today it may make sense to extend these rules to cover the
things which where created since then
Right, but if that suddenly changes, such a change also must expect that it
might need updates to all assembly implementations that implement that
interface currently. Right now, both the defacto case (any callers in the
codebase) and the explicit documentation says that it can't be called with
parameters outside of that range.
What i meant was that newly added functions should be more flexible than
these old rules. That is 2 sets of rules
1. What a caller ATM can do and expect to work (thats whats written there)
2. What an implementor of new functions should make sure is supported
With 2., do you mean if adding a new function into the same struct, or if
implementing the existing pix_abs[0][..] functions?
If you mean new implementations of the existing function interface, you
say they "should be more flexible". How flexible must they be? Is it ok to
assume h<=256 for the w=16 functions?
Gradually increasing the requirements for existing function interfaces
like you suggest is really problematic.
If we want to require more of the functions, we should document it, and
extend the checkasm test to test that new requirement - which also extends
the requirement to the existing functions. If we don't have a checkasm
test for the required behaviour, we can pretty much assume it's broken,
even in new implementations.
// Martin
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 5/5] aarch64: me_cmp: Don't do uaddlv once per iteration
On Sat, Jul 16, 2022 at 12:25:37AM +0300, Martin Storsjö wrote:
> On Fri, 15 Jul 2022, Michael Niedermayer wrote:
>
> > On Fri, Jul 15, 2022 at 10:56:03PM +0300, Martin Storsjö wrote:
> > > On Fri, 15 Jul 2022, Swinney, Jonathan wrote:
> > >
> > > > If the max height is just 16, then this should be fine. I assumed that h
> > > > could have a much higher value (>1024), but if that is not the case,
> > > > then this is a useful optimization.
> > >
> > > At least according to the me_cmp.h header, which says:
> > >
> > > /* Motion estimation:
> > > * h is limited to { width / 2, width, 2 * width },
> > > * but never larger than 16 and never smaller than 2.
> > > * Although currently h < 4 is not used as functions with
> > > * width < 8 are neither used nor implemented. */
> >
> > These rules where written with support for encoding of all
> > standard formats in mind at the time that was written.
> > today it may make sense to extend these rules to cover the
> > things which where created since then
>
> Right, but if that suddenly changes, such a change also must expect that it
> might need updates to all assembly implementations that implement that
> interface currently. Right now, both the defacto case (any callers in the
> codebase) and the explicit documentation says that it can't be called with
> parameters outside of that range.
What i meant was that newly added functions should be more flexible than
these old rules. That is 2 sets of rules
1. What a caller ATM can do and expect to work (thats whats written there)
2. What an implementor of new functions should make sure is supported
>
> Even if it's raised from the current <= 16, this particular optimization
> should be fine as long as h <= 256 - which should be fine for at least all
> current-gen mainstream codecs since, I think?
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 5/5] aarch64: me_cmp: Don't do uaddlv once per iteration
On Fri, 15 Jul 2022, Michael Niedermayer wrote:
On Fri, Jul 15, 2022 at 10:56:03PM +0300, Martin Storsjö wrote:
On Fri, 15 Jul 2022, Swinney, Jonathan wrote:
If the max height is just 16, then this should be fine. I assumed that h
could have a much higher value (>1024), but if that is not the case,
then this is a useful optimization.
At least according to the me_cmp.h header, which says:
/* Motion estimation:
* h is limited to { width / 2, width, 2 * width },
* but never larger than 16 and never smaller than 2.
* Although currently h < 4 is not used as functions with
* width < 8 are neither used nor implemented. */
These rules where written with support for encoding of all
standard formats in mind at the time that was written.
today it may make sense to extend these rules to cover the
things which where created since then
Also - if extending this, I would expect that you want other widths too.
Right now, most of the functions seem to be arranged such as [0] is w=16
and [0] is w=8. For those, for w=8, it seems to be mostly hardcoded to
only assume h=8, while the w=16 functions actually honor the h parameter.
If it ever would be relevant with h>256, that wouldn't be for the existing
w=8 or w=16 functions, but for newer functions with a larger width too.
So I think this patch is safe (which works for h up to 256), and if
someone wants to extend the interface later, that can be done.
// Martin
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 5/5] aarch64: me_cmp: Don't do uaddlv once per iteration
On Fri, 15 Jul 2022, Michael Niedermayer wrote:
On Fri, Jul 15, 2022 at 10:56:03PM +0300, Martin Storsjö wrote:
On Fri, 15 Jul 2022, Swinney, Jonathan wrote:
If the max height is just 16, then this should be fine. I assumed that h
could have a much higher value (>1024), but if that is not the case,
then this is a useful optimization.
At least according to the me_cmp.h header, which says:
/* Motion estimation:
* h is limited to { width / 2, width, 2 * width },
* but never larger than 16 and never smaller than 2.
* Although currently h < 4 is not used as functions with
* width < 8 are neither used nor implemented. */
These rules where written with support for encoding of all
standard formats in mind at the time that was written.
today it may make sense to extend these rules to cover the
things which where created since then
Right, but if that suddenly changes, such a change also must expect that
it might need updates to all assembly implementations that implement that
interface currently. Right now, both the defacto case (any callers in the
codebase) and the explicit documentation says that it can't be called with
parameters outside of that range.
Even if it's raised from the current <= 16, this particular optimization
should be fine as long as h <= 256 - which should be fine for at least all
current-gen mainstream codecs since, I think?
// Martin
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 5/5] aarch64: me_cmp: Don't do uaddlv once per iteration
On Fri, Jul 15, 2022 at 10:56:03PM +0300, Martin Storsjö wrote:
> On Fri, 15 Jul 2022, Swinney, Jonathan wrote:
>
> > If the max height is just 16, then this should be fine. I assumed that h
> > could have a much higher value (>1024), but if that is not the case,
> > then this is a useful optimization.
>
> At least according to the me_cmp.h header, which says:
>
> /* Motion estimation:
> * h is limited to { width / 2, width, 2 * width },
> * but never larger than 16 and never smaller than 2.
> * Although currently h < 4 is not used as functions with
> * width < 8 are neither used nor implemented. */
These rules where written with support for encoding of all
standard formats in mind at the time that was written.
today it may make sense to extend these rules to cover the
things which where created since then
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 5/5] aarch64: me_cmp: Don't do uaddlv once per iteration
On Fri, 15 Jul 2022, Swinney, Jonathan wrote:
If the max height is just 16, then this should be fine. I assumed that h
could have a much higher value (>1024), but if that is not the case,
then this is a useful optimization.
At least according to the me_cmp.h header, which says:
/* Motion estimation:
* h is limited to { width / 2, width, 2 * width },
* but never larger than 16 and never smaller than 2.
* Although currently h < 4 is not used as functions with
* width < 8 are neither used nor implemented. */
So with that in mind, I think this should be safe to do.
// Martin
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
