[FFmpeg-devel] [PATCH 5/5] src_movie: implement multiple outputs.

[Nicolas George]

Le tridi 3 thermidor, an CCXX, Stefano Sabatini a écrit :
> duplicated "streams"

Fixed.

> "+" as separator may conflict with "+" chars present in the specifier
> (e.g. in case it is part of the ID). So maybe some escaping/deescaping
> may be needed (add a TODO just in case).

AFAICS, + can not be necessary in a stream specifier (it can be used as part
of a positive integer, but the same integer can as well be written without
the +). If I am wrong, it would be better to notice it now and choose a
character that will not need escaping.

> > +    char *streams;
> streams_str?

stream_specs should be even better.

> > +    MovieStream *st;
> please call this "streams" (so that the reader knows it's a set), "st"
> is usually used for a single stream.

It is frequently used and would make overly long lines. I added a doxy.

> this syntax is not documented with ret > 1 (e.g "da2"). I suppose it
> is on purpose. Would be useful to support the syntax:
> b<type><stream_index>
> 
> or something mapping av_find_best_stream() facilities?

I am more or less convinced that "v<x>" yields the same result as
find_best_stream(VIDEO, x), so I do not think that would be useful. OTOH, I
do not want to take risks with backward compatibility when it is so simple
to add an internal syntax.

> > +    if (ret >= 1 && ret <= 2) {
> ret == 1 || ret == 2?

I like it better that way. Adding optional fields is easier.

> Uhm, why not? (but I suppose it will be far more complex, and thus
> should be addressed in another patch).

It was in the comments at the top. I copy it here:
>> The reason to disable looping with multiple outputs is the problem with
>> streams of slightly different duration; see what I wrote in the
>> documentation part of the concat filter in a recent patch.

> av_opt_free(&movie);

I always forget it exists. Changed. But the & was wrong.

> Nit: outlink?

If you want.

> > +    case AVMEDIA_TYPE_AUDIO:
> > +        break;
> pointless?

I like it to be there to show audio is taken care of, as opposed to
subtitles for examples.

> Any reason we don't have a unified avfilter_get_buffer_ref_from_frame()?

Patch posted. The catch is that AVFrame does not have a media_type field, it
is required as an additional argument.

> Note: this could be transformed into a shared function and used by
> *showinfo, or maybe dropped at all (since it is basically duplicating
> *showinfo functionality, which predates).

This one is only for debug; there was debug in the original code, I kept it
but I have no objection to dropping it.

> attempt

Fixed.

> Could you send the whole file, that should be more readable.

Sure.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: src_movie.c
Type: text/x-csrc
Size: 20300 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120721/09745b17/attachment.bin>
-------------- 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/20120721/09745b17/attachment.asc>