[Ffmpeg-devel] Getting rid of inlining failure warnings
Steve DeLaney
onramp123
Thu Nov 9 17:18:52 CET 2006
Apologies in advance - I've subscribed to the daily digest and don't know
yet how to reply on a specific thread.
Anyway thanks for investigating this problem. I'd like to better understand
how these changes are propagated.
I see this went out as a patch. But will these changes be pushed into the
SVN trunk?
I would like to svn update to pick up these changes. Or should we use the
patch for now?
Similarly I've got a couple of minor changes that repair build problems on
solaris. Namely
BE_16/BE_32 that was previously noted. I'd like to svn commit this change
to the trunk so I don't have to re-merge after each update.
Regards,
/steve
-----Original Message-----
From: ffmpeg-devel-bounces at mplayerhq.hu
[mailto:ffmpeg-devel-bounces at mplayerhq.hu] On Behalf Of
ffmpeg-devel-request at mplayerhq.hu
Sent: Thursday, November 09, 2006 7:47 AM
To: ffmpeg-devel at mplayerhq.hu
Subject: ffmpeg-devel Digest, Vol 20, Issue 96
Send ffmpeg-devel mailing list submissions to
ffmpeg-devel at mplayerhq.hu
To subscribe or unsubscribe via the World Wide Web, visit
http://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
or, via email, send a message with subject or body 'help' to
ffmpeg-devel-request at mplayerhq.hu
You can reach the person managing the list at
ffmpeg-devel-owner at mplayerhq.hu
When replying, please edit your Subject line so it is more specific than
"Re: Contents of ffmpeg-devel digest..."
Today's Topics:
1. Re: [PATCH] Add fact chunk to non-PCM wav (Michael Niedermayer)
2. recent breakage in libswscale build (Luca Abeni)
3. Re: recent breakage in libswscale build (Diego Biurrun)
4. Re: Getting rid of inlining failure warnings (Panagiotis Issaris)
5. Re: [PATCH] Getting rid of inlining failure warnings
(Michael Niedermayer)
6. Re: [PATCH] Add fact chunk to non-PCM wav (Michel Bardiaux)
----------------------------------------------------------------------
Message: 1
Date: Thu, 9 Nov 2006 15:49:22 +0100
From: Michael Niedermayer <michaelni at gmx.at>
Subject: Re: [Ffmpeg-devel] [PATCH] Add fact chunk to non-PCM wav
To: FFmpeg development discussions and patches
<ffmpeg-devel at mplayerhq.hu>
Message-ID: <20061109144922.GD14726 at MichaelsNB>
Content-Type: text/plain; charset=us-ascii
Hi
On Thu, Nov 09, 2006 at 01:53:08PM +0100, Michel Bardiaux wrote:
> Michael Niedermayer wrote:
> >Hi
> >
> >On Thu, Nov 09, 2006 at 11:36:13AM +0100, Michel Bardiaux wrote:
> >>Michael has reported that
> >>
> >>>>interresting, accoridng to microsofts excelent and unambigous
> >>>>documentation (not kidding ...)
> >>>> Fact Chunk
> >>>>This chunk is required for all WAVE formats other than
WAVE_FORMAT_PCM.
> >>>>It stores file
> >>>>dependent information about the contents of the WAVE data. It
> >>>>currently specifies the time length of the data in samples.
> >>>>
> >>>>so this must not be under CODEC_ID_MSGSM, also it must be a
> >>>>seperate patch as its not CODEC_ID_MSGSM specific
> >>I took this literally, hence CODEC_ID_PCM_ALAW and
> >>CODEC_ID_PCM_MULAW will get a fact chunk too.
> >
> >:)
> >
> >now just take the largest pts minus the smallest pts of any packet
> >stored and convert that by using AVStream.time_base and
> >AVCodecContext.sample_rate to the number of samples and store that in
> >the fact chunk
>
> Right, the patch does not make sense without the final update of the
> chunk, and with a *general* formula, not the one I had specialised for
> MSGSM. I'm rather rusty on the handling of timestamps, so this might
> take some time
in the write packet function
assert(avpacket->pts != AV_NOPTS_VALUE);
context->maxpts= FFMAX(context->maxpts, avpacket->pts); minpts=
context->FFMIN(context->minpts, avpacket->pts);
and in write_trailer
number_of_sample= av_rescale(context->maxpts - context->minpts,
avctx->sample_rate * (int64_t)avstream->time_base.num,
avstream->time_base.den);
untested of course but it should work approximately that way ...
> (and though I can read the lists from home, posting does not work).
hmm why?
[...]
>
> >
> >then run the regression tests and send a patch which updates the
> >checksums
>
> Not clear how to proceed here, do you mean a subsequent patch, or in
> the same?
in the fact chunk patch, sorry for being unclear
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is
------------------------------
Message: 2
Date: Thu, 09 Nov 2006 16:03:05 +0100
From: Luca Abeni <lucabe72 at email.it>
Subject: [Ffmpeg-devel] recent breakage in libswscale build
To: FFmpeg development discussions and patches
<ffmpeg-devel at mplayerhq.hu>
Message-ID: <1163084585.2890.71.camel at labeni.mm.mbigroup.it>
Content-Type: text/plain; charset="us-ascii"
Hi all,
libswscale does not currently build because of the following problem:
[...]
ranlib libavformat.a
make[1]: Leaving directory `/tmp/A/ffmpeg/libavformat'
make -C libswscale all
make[1]: Entering directory `/tmp/A/ffmpeg/libswscale'
Makefile:19: "/tmp/A/ffmpeg"/common.mak: No such file or directory
make[1]: *** No rule to make target `"/tmp/A/ffmpeg"/common.mak'. Stop.
make[1]: Leaving directory `/tmp/A/ffmpeg/libswscale'
make: *** [lib] Error 2
I think something went wrong with r6938 and the attached patch is needed (I
see that all the other makefiles do this)
Thanks,
Luca
--
____________________________________________________________________________
_
Copy this in your signature, if you think it is important:
N O W A R ! ! !
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sws_build_fix.diff
Type: text/x-patch
Size: 281 bytes
Desc: not available
Url :
http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20061109/efa154
05/sws_build_fix-0001.bin
------------------------------
Message: 3
Date: Thu, 9 Nov 2006 16:08:33 +0100
From: Diego Biurrun <diego at biurrun.de>
Subject: Re: [Ffmpeg-devel] recent breakage in libswscale build
To: FFmpeg development discussions and patches
<ffmpeg-devel at mplayerhq.hu>
Message-ID: <20061109150833.GF1961 at biurrun.de>
Content-Type: text/plain; charset=us-ascii
On Thu, Nov 09, 2006 at 04:03:05PM +0100, Luca Abeni wrote:
>
> libswscale does not currently build because of the following problem:
> [...]
> ranlib libavformat.a
> make[1]: Leaving directory `/tmp/A/ffmpeg/libavformat'
> make -C libswscale all
> make[1]: Entering directory `/tmp/A/ffmpeg/libswscale'
> Makefile:19: "/tmp/A/ffmpeg"/common.mak: No such file or directory
> make[1]: *** No rule to make target `"/tmp/A/ffmpeg"/common.mak'. Stop.
> make[1]: Leaving directory `/tmp/A/ffmpeg/libswscale'
> make: *** [lib] Error 2
>
> I think something went wrong with r6938 and the attached patch is
> needed (I see that all the other makefiles do this)
Commit it.
Diego
------------------------------
Message: 4
Date: Thu, 09 Nov 2006 16:20:49 +0100
From: Panagiotis Issaris <takis.issaris at uhasselt.be>
Subject: Re: [Ffmpeg-devel] Getting rid of inlining failure warnings
To: FFmpeg development discussions and patches
<ffmpeg-devel at mplayerhq.hu>
Message-ID: <1163085649.25490.48.camel at issaris.in.edm.luc.ac.be>
Content-Type: text/plain
Hi,
On Thu, 2006-11-09 at 15:41 +0100, Michael Niedermayer wrote:
>[...]
> rdtsc and emms should be smaller if inlined then if called, if you are
>brave and after verifying that the instructions are smaller or equal
>then a call then submit a bugreport to the gcc devels
Here's a little bit of testcode I used to verify this. If this makes sense,
I'll try to write a bugreport reporting that GCC sometimes does not inline
code claiming the function has grown to large, while inlining it would have
_decreased_ the codesize.
#include <stdio.h>
static inline long long read_time(void) {
long long l;
asm volatile( "rdtsc\n\t"
: "=A" (l)
);
return l;
}
int main()
{
long long l = read_time();
printf("%Ld\n", l);
}
#include <stdio.h>
static __attribute__ ((noinline)) long long read_time(void) {
long long l;
asm volatile( "rdtsc\n\t"
: "=A" (l)
);
return l;
}
int main() {
long long l = read_time();
printf("%Ld\n", l);
}
With a commandline equal to what FFmpeg is using here (without the FFmpeg
specific stuff):
gcc -c -I. -fomit-frame-pointer -g -Wdeclaration-after-statement -Wall
-Wno-switch -Wdisabled-optimization -Wpointer-arith -Wredundant-decls
-Winline -O3 rdtsc.c
The inlined version is indeed smaller:
size inlinerdtsc.o
text data bss dec hex filename
51 0 0 51 33 inlinerdtsc.o
size rdtsc.o
text data bss dec hex filename
68 0 0 68 44 rdtsc.o
I do not think it is specific to this short block of code, as the generated
assembly shows rdtsc being only 2 bytes long, while the call instruction by
itself already occupies 5 bytes:
Not inlined:
00000000 <read_time>:
0: 0f 31 rdtsc
2: c3 ret
3: 8d b6 00 00 00 00 lea 0x0(%esi),%esi
9: 8d bc 27 00 00 00 00 lea 0x0(%edi),%edi
00000010 <main>:
10: 8d 4c 24 04 lea 0x4(%esp),%ecx
14: 83 e4 f0 and $0xfffffff0,%esp
17: ff 71 fc pushl 0xfffffffc(%ecx)
1a: 51 push %ecx
1b: 83 ec 18 sub $0x18,%esp
1e: e8 dd ff ff ff call 0 <read_time>
23: c7 04 24 00 00 00 00 movl $0x0,(%esp)
2a: 89 44 24 04 mov %eax,0x4(%esp)
2e: 89 54 24 08 mov %edx,0x8(%esp)
32: e8 fc ff ff ff call 33 <main+0x23>
37: 83 c4 18 add $0x18,%esp
3a: 31 c0 xor %eax,%eax
3c: 59 pop %ecx
3d: 8d 61 fc lea 0xfffffffc(%ecx),%esp
40: c3 ret
Inlined:
00000000 <main>:
0: 8d 4c 24 04 lea 0x4(%esp),%ecx
4: 83 e4 f0 and $0xfffffff0,%esp
7: ff 71 fc pushl 0xfffffffc(%ecx)
a: 51 push %ecx
b: 83 ec 18 sub $0x18,%esp
e: 0f 31 rdtsc
10: 89 44 24 04 mov %eax,0x4(%esp)
14: 89 54 24 08 mov %edx,0x8(%esp)
18: c7 04 24 00 00 00 00 movl $0x0,(%esp)
1f: e8 fc ff ff ff call 20 <main+0x20>
24: 83 c4 18 add $0x18,%esp
27: 31 c0 xor %eax,%eax
29: 59 pop %ecx
2a: 8d 61 fc lea 0xfffffffc(%ecx),%esp
2d: c3 ret
> but they will probably close it with some comment like gcc cant count
> instructions in an asm (probably claiming that it is fundamentally
> impossible to do or some other similar ridiculous statement)
>
> in the meanwhile rdtsc & emms could be marked always_inline but that
> could cause other random functions to fail to be inlined (this should
> be checked a diff of "nm foobar.o" before and afterwards should give a
> definite awnser tough
I'll have a look at this, but the other patch I posted does not touch the
rdtsc & emms functions. I'm still unsure if they can influence each other.
In the sense that I might have removed some inline specifiers, which might
not have been needed if rdtsc would have been forced to inline or something
like that... Could something like that happen?
>
> also functions which are called just from one spot or just from one
> spot per filetype should be marked as always_inline
With friendly regards,
Takis
--
vCard: http://www.issaris.org/pi.vcf
Public key: http://www.issaris.org/pi.key
------------------------------
Message: 5
Date: Thu, 9 Nov 2006 16:32:56 +0100
From: Michael Niedermayer <michaelni at gmx.at>
Subject: Re: [Ffmpeg-devel] [PATCH] Getting rid of inlining failure
warnings
To: FFmpeg development discussions and patches
<ffmpeg-devel at mplayerhq.hu>
Message-ID: <20061109153255.GE14726 at MichaelsNB>
Content-Type: text/plain; charset=us-ascii
Hi
On Thu, Nov 09, 2006 at 03:47:08PM +0100, Panagiotis Issaris wrote:
> Hi,
>
> On Thu, 2006-11-09 at 14:59 +0100, Panagiotis Issaris wrote:
> > [...]
> > I'll send a new patch later in which I will send smaller patches,
> > removing the inline specifier only if no new warnings are introduced by
> > doing so.
>
> The attached patch gets rid of inlining failure warnings.
>
> Total number of warnings before the patch: 1141
> Number of inlining warning before the patch: 353
>
> Total number of warnings after the patch: 579
> Number of inlining warning after the patch: 72
>
>
> asv1.c | 6 +++---
> cavs.c | 2 +-
> ffv1.c | 4 ++--
> golomb.h | 2 +-
> h263.c | 8 ++++----
> h264.c | 6 +++---
> jpeg_ls.c | 4 ++--
> motion_est.c | 6 +++---
> motion_est_template.c | 4 ++--
> mpeg12.c | 6 +++---
> mpegvideo.c | 14 +++++++-------
> mpegvideo.h | 4 ++--
> msmpeg4.c | 8 ++++----
> snow.c | 6 +++---
> svq3.c | 2 +-
> vc1.c | 2 +-
> 16 files changed, 42 insertions(+), 42 deletions(-)
>
> Regression tests succeed. As inlining already failed, removing the
> specifier should have no performance impact (at least on all compilers
> where the same inlining warnings would have appeared...)
the warnings often are specific to function X in Y and dont mean X is never
inlined
[...]
> Index: libavcodec/ffv1.c
> ===================================================================
> --- libavcodec/ffv1.c (revision 6954)
> +++ libavcodec/ffv1.c (working copy)
> @@ -221,7 +221,7 @@
> return f->quant_table[0][(L-LT) & 0xFF] +
f->quant_table[1][(LT-T) & 0xFF] + f->quant_table[2][(T-RT) & 0xFF];
> }
>
> -static inline void put_symbol(RangeCoder *c, uint8_t *state, int v, int
is_signed){
> +static void put_symbol(RangeCoder *c, uint8_t *state, int v, int
is_signed){
the one call to put_symbol in encode_line should always be inlined all
others
should never be inlined (a noinline warper around a always_inline
put_symbol() should do the trick)
> int i;
>
> if(v){
> @@ -357,7 +357,7 @@
> }
>
> #ifdef CONFIG_ENCODERS
> -static inline int encode_line(FFV1Context *s, int w, int_fast16_t
*sample[2], int plane_index, int bits){
> +static int encode_line(FFV1Context *s, int w, int_fast16_t *sample[2],
int plane_index, int bits){
used only once per fileformat case -> always_inline
and try attriute(flatten) or change gccs inlining limits if needed, but the
bits parameter is a constant and used in the inner loop, the same is true
for jpeg_ls, so these may be faster if inlined (and nothing else is outlined
by gcc)
[...]
> Index: libavcodec/jpeg_ls.c
> ===================================================================
> --- libavcodec/jpeg_ls.c (revision 6954)
> +++ libavcodec/jpeg_ls.c (working copy)
> @@ -288,7 +288,7 @@
> /**
> * Decode one line of image
> */
> -static inline void ls_decode_line(JLSState *state, MJpegDecodeContext *s,
void *last, void *dst, int last2, int w, int stride, int comp, int bits){
> +static void ls_decode_line(JLSState *state, MJpegDecodeContext *s, void
*last, void *dst, int last2, int w, int stride, int comp, int bits){
used just once per fileformat -> always_inline
[...]
> @@ -577,7 +577,7 @@
> /**
> * Encode one line of image
> */
> -static inline void ls_encode_line(JLSState *state, PutBitContext *pb,
void *last, void *cur, int last2, int w, int stride, int comp, int bits){
> +static void ls_encode_line(JLSState *state, PutBitContext *pb, void
*last, void *cur, int last2, int w, int stride, int comp, int bits){
used just once per fileformat -> always_inline
[...]
> Index: libavcodec/mpegvideo.c
> ===================================================================
> --- libavcodec/mpegvideo.c (revision 6954)
> +++ libavcodec/mpegvideo.c (working copy)
> @@ -3438,7 +3438,7 @@
> * @param pic_op qpel motion compensation function (average or put
normally)
> * the motion vectors are taken from s->mv and the MV type from
s->mv_type
> */
> -static inline void MPV_motion(MpegEncContext *s,
> +static void MPV_motion(MpegEncContext *s,
this and the lowres case ok
btw you could try to factor the s->mv[dir] in these functions out, like:
mvdir= s->mv[dir] at the top of the functions, this might make them faster
or
even pass mvdir as a parameter to them, as dir is a constant in all calls
[...]
> @@ -4121,7 +4121,7 @@
>
> #ifdef CONFIG_ENCODERS
>
> -static inline void dct_single_coeff_elimination(MpegEncContext *s, int n,
int threshold)
> +static void dct_single_coeff_elimination(MpegEncContext *s, int n, int
threshold)
noone uses single_coeff_elimination so ok
> {
> static const char tab[64]=
> {3,2,2,1,1,1,1,1,
> @@ -4170,7 +4170,7 @@
> else s->block_last_index[n]= -1;
> }
>
> -static inline void clip_coeffs(MpegEncContext *s, DCTELEM *block, int
last_index)
> +static void clip_coeffs(MpegEncContext *s, DCTELEM *block, int
last_index)
called rarely so ok
[...]
> @@ -4636,7 +4636,7 @@
> put_bits(pb, bits, be2me_16(srcw[words])>>(16-bits));
> }
>
> -static inline void copy_context_before_encode(MpegEncContext *d,
MpegEncContext *s, int type){
> +static void copy_context_before_encode(MpegEncContext *d, MpegEncContext
*s, int type){
hmm, iam unsure, if this should be inlined or not, id say leave it
[...]
> @@ -4662,7 +4662,7 @@
> d->dquant= s->dquant;
> }
>
> -static inline void copy_context_after_encode(MpegEncContext *d,
MpegEncContext *s, int type){
> +static void copy_context_after_encode(MpegEncContext *d, MpegEncContext
*s, int type){
hmm, iam unsure, if this should be inlined or not, id say leave it
[...]
> @@ -4699,7 +4699,7 @@
> d->qscale= s->qscale;
> }
>
> -static inline void encode_mb_hq(MpegEncContext *s, MpegEncContext
*backup, MpegEncContext *best, int type,
> +static void encode_mb_hq(MpegEncContext *s, MpegEncContext *backup,
MpegEncContext *best, int type,
> PutBitContext pb[2], PutBitContext pb2[2],
PutBitContext tex_pb[2],
> int *dmin, int *next_block, int motion_x, int
motion_y)
ok, this is called from too many spots
ill look at the stuff after mpegvideo* later, the above should be
dealt with and benchmarked first IMHO
to benchmark START/STOP_TIMER at appropriate places should be used
appropriate is not to close and not to far away from the code
one thing you could try is around avcodec_en/decode_video() ...
but that might be too far away to detect small differences reliably
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is
------------------------------
Message: 6
Date: Thu, 09 Nov 2006 16:46:47 +0100
From: Michel Bardiaux <mbardiaux at mediaxim.be>
Subject: Re: [Ffmpeg-devel] [PATCH] Add fact chunk to non-PCM wav
To: FFmpeg development discussions and patches
<ffmpeg-devel at mplayerhq.hu>
Message-ID: <45534D67.4040003 at mediaxim.be>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Michael Niedermayer wrote:
> Hi
>
> On Thu, Nov 09, 2006 at 01:53:08PM +0100, Michel Bardiaux wrote:
>> Michael Niedermayer wrote:
>>> Hi
>>>
>>> On Thu, Nov 09, 2006 at 11:36:13AM +0100, Michel Bardiaux wrote:
>>>> Michael has reported that
>>>>
>>>>>> interresting, accoridng to microsofts excelent and unambigous
>>>>>> documentation
>>>>>> (not kidding ...)
>>>>>> Fact Chunk
>>>>>> This chunk is required for all WAVE formats other than
WAVE_FORMAT_PCM.
>>>>>> It stores file
>>>>>> dependent information about the contents of the WAVE data. It
currently
>>>>>> specifies the time length of the
>>>>>> data in samples.
>>>>>>
>>>>>> so this must not be under CODEC_ID_MSGSM, also it must be a seperate
>>>>>> patch
>>>>>> as its not CODEC_ID_MSGSM specific
>>>> I took this literally, hence CODEC_ID_PCM_ALAW and CODEC_ID_PCM_MULAW
>>>> will get a fact chunk too.
>>> :)
>>>
>>> now just take the largest pts minus the smallest pts of any packet
stored
>>> and convert that by using AVStream.time_base and
AVCodecContext.sample_rate
>>> to the number of samples and store that in the fact chunk
>> Right, the patch does not make sense without the final update of the
>> chunk, and with a *general* formula, not the one I had specialised for
>> MSGSM. I'm rather rusty on the handling of timestamps, so this might
>> take some time
>
> in the write packet function
> assert(avpacket->pts != AV_NOPTS_VALUE);
> context->maxpts= FFMAX(context->maxpts, avpacket->pts);
> context->minpts= FFMIN(context->minpts, avpacket->pts);
>
> and in write_trailer
> number_of_sample= av_rescale(context->maxpts - context->minpts,
avctx->sample_rate * (int64_t)avstream->time_base.num,
avstream->time_base.den);
>
> untested of course but it should work approximately that way ...
Thanks, will try that.
>
>
>> (and though I can read the lists from home, posting does
>> not work).
>
> hmm why?
The address I'm subscribed on, directs the list traffic to a corporate
IMAP server. This has SSL access, hence I can access it from home thru
the firewall.
I could post from home, but I think the lists are now subscriber-only,
and if I subscribe from 2 places I would get the traffic at 2 places,
which is wasteful. Or is there a way to subscribe for posting only?
>
>
> [...]
>
>>> then run the regression tests and send a patch which updates the
checksums
>> Not clear how to proceed here, do you mean a subsequent patch, or in the
>> same?
>
> in the fact chunk patch, sorry for being unclear
>
> [...]
--
Michel Bardiaux
R&D Director
T +32 [0] 2 790 29 41
F +32 [0] 2 790 29 02
E mailto:mbardiaux at mediaxim.be
Mediaxim NV/SA
Vorstlaan 191 Boulevard du Souverain
Brussel 1160 Bruxelles
http://www.mediaxim.com/
------------------------------
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel at mplayerhq.hu
http://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
End of ffmpeg-devel Digest, Vol 20, Issue 96
********************************************
More information about the ffmpeg-devel
mailing list