[FFmpeg-devel] [PATCH] Common ACELP routines (2/3) - filters

[Michael Niedermayer]

On Fri, Apr 25, 2008 at 09:29:01AM +0100, Robert Swain wrote:
> On 25 Apr 2008, at 08:40, Vladimir Voroshilov wrote:
> > Michael Niedermayer wrote:
> >> On Fri, Apr 25, 2008 at 08:22:15AM +0700, Vladimir Voroshilov wrote:
> >>> On Fri, Apr 25, 2008 at 1:17 AM, Michael Niedermayer <michaelni at gmx.at 
> >>> > wrote:
> >>>> On Fri, Apr 25, 2008 at 12:07:15AM +0700, Vladimir Voroshilov  
> >>>> wrote:
> >>>>> On Thu, Apr 24, 2008 at 10:13 AM, Michael Niedermayer <michaelni at gmx.at 
> >>>>> > wrote:
> >>>> [...]
> >>>>>>>>> +        filter_data[10+n] = out[n] = sum;
> >>>>>>>>
> >>>>>>>> This duplicated storeage is unacceptable.
> >>>>>>>
> >>>>>>> First for all assigned to filter data values will be used in  
> >>>>>>> loop later.
> >>>>>>> Thus filter_data can not be eliminated.
> >>>>>>> I can't use "out" instead of it due to necessary 10 items
> >>>>>>> with data from previous subframe at top).
> >>>>>>> Extending out with 10 items at top will require another  
> >>>>>>> temporary buffer
> >>>>>>> one memcpy somewhere later (because i will not be able to use  
> >>>>>>> output buffer
> >>>>>>> directly).
> >>>>>>
> >>>>>> The double write is definitly useless after the first 10  
> >>>>>> iterations as
> >>>>>> after that you can just work in the out buffer.
> >>>>>>
> >>>>>> foobar_filter(filter_data+10, 10);
> >>>>>> memcpy(out, filter_data+10, 10);
> >>>>>> foobar_filter(out+10, N-10);
> >>>>>>
> >>>>>> should work fine and will for large N (dunno how large it is,  
> >>>>>> so maybe
> >>>>>> this isnt worth it ...) be faster. Also it allows filter_data  
> >>>>>> to be smaller.
> >>>>>
> >>>>> ... and code will look like :(
> >>>>>
> >>>>> if(foobar_filter(filter_data+10, 10)!=OVERFLOW)
> >>>>> {
> >>>>>  memcpy(out, filter_data+10, 10);
> >>>>>  if(foobar_filter(out+10, N-10)==OVERFLOW)
> >>>>>  {
> >>>>>     for(i=0;i<len;i++) out>>=2;
> >>>>>     foobar_filter(filter_data+10, 10);
> >>>>>     memcpy(out, filter_data+10, 10);
> >>>>>     foobar_filter(out+10, N-10);
> >>>>>  }
> >>>>> }
> >>>>> else
> >>>>> {
> >>>>>     for(i=0;i<len;i++) out>>=2;
> >>>>>     foobar_filter(filter_data+10, 10);
> >>>>>     memcpy(out, filter_data+10, 10);
> >>>>>     foobar_filter(out+10, N-10);
> >>>>> }
> >>>>
> >>>> for(;;){
> >>>>    overflow= foobar_filter(filter_data+10, 10);
> >>>>
> >>>>    memcpy(out, filter_data+10, 10);
> >>>>    overflow|= foobar_filter(out+10, N-10);
> >>>>    if(!overflow)
> >>>>        break;
> >>>>
> >>>>    for(i=0;i<len;i++) out>>=2;
> >>>> }
> >>>
> >>> This will change filter_data even if overflow occuried.
> >>> Which cause wrong synthesis result on second iteration.
> >>> Current code on overflow case just downscales
> >>> excitation signal (without touching filter data).
> >>
> >> well its a matter of adding if(!overflow)
> >>
> >> Also note that a memcpy is likely faster than the one by one  
> >> element writing
> >> in the loop. So if you dislike spliting it in 2 like above, then a  
> >> seperate
> >> memcpy is definitly prefered over the double writing.
> >
> > I've split it in two routines.
> > first is alike your's foobar_filter.
> >
> > Second routine calls previous one
> > twice - for first 10 samples and the remaining.
> >
> > I've also moved output parameters to top.
> 
> I'm slightly confused by what the problem is here. I was out last  
> night else I would have commented earlier.
> 
> My AMR code uses a sample buffer that is AMR_SUBFRAME_SIZE +  
> LP_FILTER_ORDER samples. I store the appropriate filtered speech  
> samples from the previous subframe at the beginning of the buffer  
> (first 10 elements) and use the rest for the calculations for the  
> current subframe.

Yes thats also what i thought at first but vladimirs insistance made
me belive that the output of this function would be the final audio
samples returned from "decode_audio". In which cases one of course
cannot have 10 samples before it.
Now after looking at the actual code its clear that there are always
some other filters applied afterwards. So i agree, none of this makes
sense, no double writing, no special case for the first "10" samples
nor a memcpy() should be needed. Except a memcpy to move the first
"10" samples to the buffer start.

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080425/95e40fdb/attachment.pgp>