[FFmpeg-devel] [PATCH 2/2] lavfi: add amerge audio filter.

[Nicolas George]

L'octidi 8 nivôse, an CCXX, Stefano Sabatini a écrit :
> unnecessary?

Indeed.

> these may benefit from a doxy

Added.

> consistency nit: struct amerge_context -> AMergeContext

Done.

> unnecessary?

Removed.

> maybe mention the value of SWR_CH_MAX

Done.

> nit: out_no is confusing, indeed I can't even fid a better name, maybe you can

Done: out_ch_number.

> here it will be extremely helpful an INFO log of the kind:
> in_chl1:%s in_chl2:%s -> out_chl:%s

Done. A possible enhancement: print the detailed route in verbose mode.

> What when one stream ends? I see two possible policies, the filter may
> continue to stream and fill with zeros the finished channels, or
> return EOF when the first one finishes (configurable?).

The current code stops with the shortest. I stated it in the doc. Let's
leave choosing the other behaviour as a future improvement, shall we?

> please add some doxy

Done.

> suggestion: inlink_id (more readable to me)

Done: input_number.

> ibuf -> inbuf?

Done.

> Is AV_PERM_PRESERVE really necessary?

In fact, I am not really sure what these permissions mean. I do not want
anyone to fiddle with the samples in the buffer queue, especially their
size; thus PRESERVE. OTOH, I have no idea in what circumstances something
could possibly modify them.

> That seems weird to me, but I'm possibly naive about what the compiler
> is expected to do, I'd expect that a const int bps in copy_samples()
> would have the same effect...

The compiler can see by itself that bps is const in the body of the
function, but it still needs to produce code that will work with any value
of bps. OTOH, when called with a compile-time constant as bps, it produce
code that is specifically optimized for that constant. You'll notice that
copy_samples is declared with the inline directive: each of the calls
produce an instance of the code, one specialized for 1bps, one specialized
for 2bps, one specialized for 4bps and one generic.

I did not look at the assembly code, but I expect most of the difference
happens in memcpy: it will be completely unrolled in the specialized
versions. It would probably be even faster if I could find a way to
guarantee to the compiler that ins[i] and *outs are properly aligned.

> nit+: missing final dot

Fixed. New patch incoming.

> And thanks for taking care to write this very important filter.

No problem, that was interesting.

By the way, if you have time, I would really like your advice about the
ideas I posted on 22 Dec 2011 14:32:09 about making it possible for input
and output pads to be arrays.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20111229/aa1b4931/attachment.asc>