[FFmpeg-devel] [PATCH] lavfi: add concat filter.

[Nicolas George]

Le duodi 2 thermidor, an CCXX, Stefano Sabatini a écrit :
> Maybe "n", so there is no conflict when Subtitles will be added (if
> ever).

Good point. Changed.

>	 Also having long aliases may help script readability.

I must say I am not very fond of them.

> output video streams, that is also the number of video streams in each
> segment.

> output audio streams, that is also the number of audio streams in each
> segment.

More clear indeed. Changed.

> Why not:
> movie=part1.mp4, scale=512:288 [v1] ; amovie=part1.mp4 [a1] ;
> movie=part2.mp4, scale=512:288 [v2] ; amovie=part2.mp4 [a2] ;
> [v1][a1] [v2][a2] concat=v=1:a=1 [outv][outa]
> ?

The point of this example is to insist on the problem of the desync at the
stitch:

> > +Note that a desync will happen at the stitch if the audio and video streams
> > +do not have exactly the same duration in the first file.

> maybe I should rename avf_showwaves -> f_showwaves (or the other way
> around, f_concat -> avf_concat).

I am not sure: a filter that transforms audio into video is not the same
thing as a filter that can operate either on audio or video (or both).

> Nit: this is ambiguous, what about nb_segment_streams (or simply add a ///< doxy

Doxy added.

> nit: nb_segments

Ok.

> please explain in doxy what these fields mean

Done.

> comment: number of active input segments, or number of active inputs?

Doxy added.

> Also I suppose a segment is considered "active" when no corresponding
> stream already terminated, right?

For segments, there is only the current one. Inputs in the current segment
are active until they reach EOF. When all inputs in the current segment have
reached EOF, we go to the next.

> Nit+: one line per option?

I find it less readable, the lines are much too long, and as such either
wrapped at any odd place or need scrolling.

> I'd find more logical to have init before uninit.

I moved init at the end to avoid forward declarations of the function
pointers inserted in the pads. I moved uninit too.

> Nit++: "str" is usually associated to strings, I suggest to
> nb_str->nb_streams and str->stream_idx and move them to the internal
> loop where they're used.

That would force me to go beyond the 80-chars limit or wrap lines at a lot
of places. There is really no ambiguity here.

> maybe add a comment here along the lines: set the output stream formats

Added.

> Uhm... why ff_all_formats(AVMEDIA_TYPE_VIDEO) is not under an if
> (VIDEO) block?

Good catch, fixed.

> leak if rates && !layouts

Hopefully fixed.

> comment: set the formats for each corresponding segment input

Added.

> what would be the advantage of such mode?

Michael is playing with vf_scale and vf_pad to be able to digest resolution
changes. If he does succeeds, the output of concat could take advantage of
it.

> I suggest: "Input link %s parameters size:%dx%d sar:%d/%d mismatch output link %s parameters size:%dx%d sar:%d/%d mismatch\n"

Done.

> when can it happen than sample_rate is not set?

If it's video, at least.

> I'm slightly against the use of a nominal phrase for a function name,
> what about "process_frame()"?

Ok.

> Nit+++: func(...) { } 
> saves some lines and looks more readable (to me).

Ok. We really should add common functions.

> nit++: tb or time_base seems less ambiguous at first reading (silencetb, streamtb, streamb?)

Changed into rate_tb.

> Concatenate ...

Changed.

Also: added get_*_buffer callbacks, fixed permissions (REUSE2 rejected), and
used a more efficient way of identifying a link. New patch incoming.

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/20120720/5bf531f4/attachment.asc>