Re: [FFmpeg-devel] [PATCH 1/4] avutil/tablegen: add tablegen libm compatibility shims

[Ganesh Ajjanagadde]

On Sat, Nov 28, 2015 at 4:23 PM, Ganesh Ajjanagadde  wrote:
> On Sat, Nov 28, 2015 at 3:19 PM, Derek Buitenhuis
>  wrote:
>> On 11/28/2015 7:51 PM, Ganesh Ajjanagadde wrote:
>>> In principle of course, e.g with more ifdefry, configure, or something
>>> of that sort. I do not believe this is what he meant.
>>
>> I will await his reply to clarify, then. Perhaps do not write so 
>> matter-of-factly.
>>
>>> In any case, the point is moot - the implementations are not broken -
>>> if they are, avutil/libm needs fixing as well.
>>
>> Now, yes. It would certainly be nice if this was noted somewhere in the 
>> patch notes
>> (not necessarily in the commit messages). There are quite a few threads on 
>> the list
>> from you now regarding tablegen, and it can be a pain in the butt / tedious 
>> to figure
>> out how the current version differs from the N others.
>
> I assumed from the cover letter that this supersedes all such prior
> stuff. Anyway, will attempt but can't guarantee improvements in
> future.
>
>>
 Anyway, shouldn't this patce dropped, since 
 79abf2d0ded860acf505de22c4f7a750e5e98446
 removed hardcoded tables anyway?
>>>
>>> No, if you actually read even the first line of the commit message,
>>> you can see it was only for aac, not for this patch series.
>>
>> Indeed, I misread, and missed the 'aac_'. You could certainly be less of
>> an ass about it, though.
>
> Sorry about that. Please do continue to call me out on such things; I
> do not like unpleasantness on ffmpeg-devel.

pushed, thanks

>
>>
>> - Derek
>> ___
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 1/4] avutil/tablegen: add tablegen libm compatibility shims

[Ganesh Ajjanagadde]

This is useful for build-time table generation (--enable-hardcoded-tables),
by providing compat shims for hosts that have broken libms.

This file is deliberately kept minimal; functions can always be added on
an as-needed basis.

Reviewed-by: Clément Bœsch 
Reviewed-by: Ronald S. Bultje 
Signed-off-by: Ganesh Ajjanagadde 
---
 libavutil/tablegen.h | 53 
 1 file changed, 53 insertions(+)
 create mode 100644 libavutil/tablegen.h

diff --git a/libavutil/tablegen.h b/libavutil/tablegen.h
new file mode 100644
index 000..f81b46b
--- /dev/null
+++ b/libavutil/tablegen.h
@@ -0,0 +1,53 @@
+/*
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+/**
+ * @file
+ * Compatibility libm for table generation files
+ */
+
+#ifndef AVUTIL_TABLEGEN_H
+#define AVUTIL_TABLEGEN_H
+
+// we lack some functions on all host platforms, and we don't care about
+// performance and/or strict ISO C semantics as it's performed at build time
+static inline double ff_cbrt(double x)
+{
+return x < 0 ? -pow(-x, 1.0 / 3.0) : pow(x, 1.0 / 3.0);
+}
+#define cbrt ff_cbrt
+
+static inline double ff_rint(double x)
+{
+return x >= 0 ? floor(x + 0.5) : ceil(x - 0.5);
+}
+#define rint ff_rint
+
+static inline long long ff_llrint(double x)
+{
+return rint(x);
+}
+#define llrint ff_llrint
+
+static inline long ff_lrint(double x)
+{
+return rint(x);
+}
+#define lrint ff_lrint
+
+#endif /* AVUTIL_TABLEGEN_H */
-- 
2.6.2

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/4] avutil/tablegen: add tablegen libm compatibility shims

[Derek Buitenhuis]

On 11/28/2015 5:03 PM, Ganesh Ajjanagadde wrote:
> +static inline double ff_cbrt(double x)
> +{
> +return x < 0 ? -pow(-x, 1.0 / 3.0) : pow(x, 1.0 / 3.0);
> +}
> +#define cbrt ff_cbrt

Didn't Clément say to not pollute the global namespace like this?

- Derek
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/4] avutil/tablegen: add tablegen libm compatibility shims

[Ganesh Ajjanagadde]

On Sat, Nov 28, 2015 at 1:31 PM, Derek Buitenhuis
 wrote:
> On 11/28/2015 5:03 PM, Ganesh Ajjanagadde wrote:
>> +static inline double ff_cbrt(double x)
>> +{
>> +return x < 0 ? -pow(-x, 1.0 / 3.0) : pow(x, 1.0 / 3.0);
>> +}
>> +#define cbrt ff_cbrt
>
> Didn't Clément say to not pollute the global namespace like this?

There are 2 answers to this. For reference, here is the quote by Clement:

"it's pretty fine with me to use a simple hack like this, but then i'd
argue it's better if the global c namespace is not polluted with openly
broken implementations: just name the macro differently (ffrint,
simplerint, or whatever) to make sure someone doesn't end up using it in a
different context where negative values matter (and where the issue won't
get detected quickly)"

1. These implementations are not openly broken, since I actually
copied stuff from avutil/libm here, based on this review. Thus, that
aspect does not apply.
2. Clement's idea AFAIK does not work, since the names must be
identical to the standard C names for the build to work with/without
hardcoded tables. BTW, this was also pointed out by me in a reply to
that quote.

>
> - Derek
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/4] avutil/tablegen: add tablegen libm compatibility shims

[Ganesh Ajjanagadde]

On Sat, Nov 28, 2015 at 2:45 PM, Derek Buitenhuis
 wrote:
> On 11/28/2015 7:33 PM, Ganesh Ajjanagadde wrote:
>> 2. Clement's idea AFAIK does not work, since the names must be
>> identical to the standard C names for the build to work with/without
>> hardcoded tables.
>
> ... yes it can. It doesn't work without more work, but it it sure can
> work.

In principle of course, e.g with more ifdefry, configure, or something
of that sort. I do not believe this is what he meant.
In any case, the point is moot - the implementations are not broken -
if they are, avutil/libm needs fixing as well.

>
> Anyway, shouldn't this patce dropped, since 
> 79abf2d0ded860acf505de22c4f7a750e5e98446
> removed hardcoded tables anyway?

No, if you actually read even the first line of the commit message,
you can see it was only for aac, not for this patch series.

>
> - Derek
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/4] avutil/tablegen: add tablegen libm compatibility shims

[Derek Buitenhuis]

On 11/28/2015 7:51 PM, Ganesh Ajjanagadde wrote:
> In principle of course, e.g with more ifdefry, configure, or something
> of that sort. I do not believe this is what he meant.

I will await his reply to clarify, then. Perhaps do not write so 
matter-of-factly.

> In any case, the point is moot - the implementations are not broken -
> if they are, avutil/libm needs fixing as well.

Now, yes. It would certainly be nice if this was noted somewhere in the patch 
notes
(not necessarily in the commit messages). There are quite a few threads on the 
list
from you now regarding tablegen, and it can be a pain in the butt / tedious to 
figure
out how the current version differs from the N others.

>> Anyway, shouldn't this patce dropped, since 
>> 79abf2d0ded860acf505de22c4f7a750e5e98446
>> removed hardcoded tables anyway?
> 
> No, if you actually read even the first line of the commit message,
> you can see it was only for aac, not for this patch series.

Indeed, I misread, and missed the 'aac_'. You could certainly be less of
an ass about it, though.

- Derek
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/4] avutil/tablegen: add tablegen libm compatibility shims

[Derek Buitenhuis]

On 11/28/2015 7:33 PM, Ganesh Ajjanagadde wrote:
> 2. Clement's idea AFAIK does not work, since the names must be
> identical to the standard C names for the build to work with/without
> hardcoded tables.

... yes it can. It doesn't work without more work, but it it sure can
work.

Anyway, shouldn't this patce dropped, since 
79abf2d0ded860acf505de22c4f7a750e5e98446
removed hardcoded tables anyway?

- Derek
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/4] avutil/tablegen: add tablegen libm compatibility shims

[Ganesh Ajjanagadde]

On Sat, Nov 28, 2015 at 3:19 PM, Derek Buitenhuis
 wrote:
> On 11/28/2015 7:51 PM, Ganesh Ajjanagadde wrote:
>> In principle of course, e.g with more ifdefry, configure, or something
>> of that sort. I do not believe this is what he meant.
>
> I will await his reply to clarify, then. Perhaps do not write so 
> matter-of-factly.
>
>> In any case, the point is moot - the implementations are not broken -
>> if they are, avutil/libm needs fixing as well.
>
> Now, yes. It would certainly be nice if this was noted somewhere in the patch 
> notes
> (not necessarily in the commit messages). There are quite a few threads on 
> the list
> from you now regarding tablegen, and it can be a pain in the butt / tedious 
> to figure
> out how the current version differs from the N others.

I assumed from the cover letter that this supersedes all such prior
stuff. Anyway, will attempt but can't guarantee improvements in
future.

>
>>> Anyway, shouldn't this patce dropped, since 
>>> 79abf2d0ded860acf505de22c4f7a750e5e98446
>>> removed hardcoded tables anyway?
>>
>> No, if you actually read even the first line of the commit message,
>> you can see it was only for aac, not for this patch series.
>
> Indeed, I misread, and missed the 'aac_'. You could certainly be less of
> an ass about it, though.

Sorry about that. Please do continue to call me out on such things; I
do not like unpleasantness on ffmpeg-devel.

>
> - Derek
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel