[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