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

Stefano Sabatini stefasab at gmail.com
Mon Jul 23 02:07:20 CEST 2012


On date Saturday 2012-07-21 01:15:29 +0200, Nicolas George encoded:
> 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.

A stream spec may contain a program_id or a stream_id, I don't know if
they could contain the '+' character.

I suppose for the moment we can keep '+' and de-escape the spec list
in case of need (e.g. by using av_get_token() instead of av_strtok()).

(Unrelated note, may be useful to match the language of the stream in
case it is specified).

> 
> > > +    char *streams;
> > streams_str?
> 
> stream_specs should be even better.

yes

> 
> > > +    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.

Do as you prefer.

> 
> > attempt
> 
> Fixed.
> 
> > Could you send the whole file, that should be more readable.
> 
> Sure.
> 
> Regards,
> 
> -- 
>   Nicolas George

[...]
> typedef struct {
>     /* common A/V fields */
>     const AVClass *class;
>     int64_t seek_point;   ///< seekpoint in microseconds
>     double seek_point_d;
>     char *format_name;
>     char *file_name;

>     char *stream_specs;

doxy may help

>     int stream_index; /**< for compatibility */
>     int loop_count;
> 
>     AVFormatContext *format_ctx;
>     int eof;
>     AVPacket pkt, pkt0;
>     AVFrame *frame;   ///< video frame to store the decoded images in
> 

>     int max_stream_index;

and here

>     MovieStream *st; /**< array of all streams, one per output */

>     int *out_index;

and here too

> } MovieContext;
[...]
> static AVStream *find_stream(void *log, AVFormatContext *avf, const char *spec)
> {
>     int i, ret, already = 0, stream_id = -1;
>     char type_char, dummy;
>     AVStream *found = NULL;
>     enum AVMediaType type;
> 
>     ret = sscanf(spec, "d%[av]%d%c", &type_char, &stream_id, &dummy);
>     if (ret >= 1 && ret <= 2) {
>         type = type_char == 'v' ? AVMEDIA_TYPE_VIDEO : AVMEDIA_TYPE_AUDIO;
>         ret = av_find_best_stream(avf, type, stream_id, -1, NULL, 0);
>         if (ret < 0) {
>             av_log(log, AV_LOG_ERROR, "No %s stream with index '%d' found\n",
>                    av_get_media_type_string(type), stream_id);
>             return NULL;
>         }
>         return avf->streams[ret];
>     }
>     for (i = 0; i < avf->nb_streams; i++) {
>         ret = avformat_match_stream_specifier(avf, avf->streams[i], spec);
>         if (ret < 0) {
>             av_log(log, AV_LOG_ERROR,
>                    "Invalid stream specifier \"%s\"\n", spec);
>             return NULL;
>         }
>         if (!ret)
>             continue;
>         if (avf->streams[i]->discard != AVDISCARD_ALL) {
>             already++;
>             continue;
>         }
>         if (found) {
>             av_log(log, AV_LOG_WARNING,
>                    "Ambiguous stream specifier \"%s\", using #%d\n", spec, i);
>             break;
>         }
>         found = avf->streams[i];
>     }
>     if (!found) {
>         av_log(log, AV_LOG_WARNING, "Stream specifier \"%s\" %s\n", spec,

>                already ? "matched only already used streams" :

matched only by already used stream?

>                          "did not match any stream");
>         return NULL;
>     }
>     if (found->codec->codec_type != AVMEDIA_TYPE_VIDEO &&
>         found->codec->codec_type != AVMEDIA_TYPE_AUDIO) {
>         av_log(log, AV_LOG_ERROR, "Stream specifier \"%s\" matched a %s stream,"
>                "currently unsupported by libavfilter.\n", spec,

nit++: drop the final dot, should be more consistent with most libav* messages

>                av_get_media_type_string(found->codec->codec_type));
>         return NULL;
>     }
>     return found;
> }
> 
> static int open_stream(void *log, MovieStream *st)
> {
>     AVCodec *codec;
>     int ret;
> 
>     codec = avcodec_find_decoder(st->st->codec->codec_id);
>     if (!codec) {
>         av_log(log, AV_LOG_ERROR, "Failed to find any codec\n");
>         return AVERROR(EINVAL);
>     }
> 
>     if ((ret = avcodec_open2(st->st->codec, codec, NULL)) < 0) {
>         av_log(log, AV_LOG_ERROR, "Failed to open codec\n");
>         return ret;
>     }

Unrelated, and feel free to not reply, I wonder how we should specify
the options for the codec/format (this is especially relevant for a
sink movie with multiple inputs). We could specify the options using
an AVDictionary -> string serializer (like implemented in a recent
patch), but then we need to specify the stream/format to which the
options apply.

A possibility would be to apply stream specifier to options, for
example:
movie=a=1:v=1:
fopts='probesize=1M, cryptokey=0xdeadbeef':
copts='codec:a0=rawaudio, sample_fmt:a=u8, codec:v0=rawvideo, pix_fmt:v0=rgb24'

>     return 0;
> }
> 
> static av_cold int movie_init(AVFilterContext *ctx, const char *args)
> {
>     MovieContext *movie = ctx->priv;
>     AVInputFormat *iformat = NULL;
>     int64_t timestamp;
>     int nb_streams, ret, i;
>     char default_streams[16], *stream_specs, *spec, *cursor;
>     char name[16];
>     AVStream *st;
> 
>     movie->class = &movie_class;
>     av_opt_set_defaults(movie);
> 
>     if (args)
>         movie->file_name = av_get_token(&args, ":");
>     if (!movie->file_name || !*movie->file_name) {
>         av_log(ctx, AV_LOG_ERROR, "No filename provided!\n");
>         return AVERROR(EINVAL);
>     }
> 
>     if (*args++ == ':' && (ret = av_set_options_string(movie, args, "=", ":")) < 0) {
>         av_log(ctx, AV_LOG_ERROR, "Error parsing options string: '%s'\n", args);
>         return ret;
>     }
> 
>     movie->seek_point = movie->seek_point_d * 1000000 + 0.5;
> 
>     stream_specs = movie->stream_specs;
>     if (!stream_specs) {
>         snprintf(default_streams, sizeof(default_streams), "d%c%d",
>                  !strcmp(ctx->filter->name, "amovie") ? 'a' : 'v',
>                  movie->stream_index);
>         stream_specs = default_streams;
>     }
>     for (cursor = stream_specs, nb_streams = 1; *cursor; cursor++)
>         if (*cursor == '+')
>             nb_streams++;
> 

>     if (movie->loop_count != 1 && nb_streams != 1) {
>         av_log(ctx, AV_LOG_ERROR, "Can not loop with several streams.\n");

nit+: drop .

Also since this is a PAWE, maybe: "Loop with several streams is currently unsupported"

>         return AVERROR_PATCHWELCOME;
>     }
> 
[...] 
> static AVFilterBufferRef *frame_to_buf(enum AVMediaType type, AVFrame *frame,
>                                        AVFilterLink *outlink)
> {
>     AVFilterBufferRef *buf = NULL, *copy;
> 

>     switch (type) {
>     case AVMEDIA_TYPE_VIDEO:
>         buf = avfilter_get_video_buffer_ref_from_frame(frame,
>                                                        AV_PERM_WRITE |
>                                                        AV_PERM_PRESERVE |
>                                                        AV_PERM_REUSE2);
>         break;
>     case AVMEDIA_TYPE_AUDIO:
>         buf = avfilter_get_audio_buffer_ref_from_frame(frame,
>                                                        AV_PERM_WRITE |
>                                                        AV_PERM_PRESERVE |
>                                                        AV_PERM_REUSE2);
>         break;
>     }

can be simplified with the newly added avfilter_get_buffer_ref_from_frame()

>     if (!buf)
>         return NULL;
>     buf->pts = av_frame_get_best_effort_timestamp(frame);
>     copy = ff_copy_buffer_ref(outlink, buf);
>     if (!copy)
>         return NULL;
>     buf->buf->data[0] = NULL; /* it belongs to the frame */
>     avfilter_unref_buffer(buf);
>     return copy;
> }
> 
[...]
> static int rewind_file(AVFilterContext *ctx)
> {
>     MovieContext *movie = ctx->priv;
>     int64_t timestamp = movie->seek_point;
>     int ret, i;
> 
>     if (movie->format_ctx->start_time != AV_NOPTS_VALUE)
>         timestamp += movie->format_ctx->start_time;
>     ret = av_seek_frame(movie->format_ctx, -1, timestamp, AVSEEK_FLAG_BACKWARD);
>     if (ret < 0) {
>         av_log(ctx, AV_LOG_ERROR, "Unable to loop: %s\n", av_err2str(ret));
>         movie->loop_count = 1; /* do not try again */
>         return ret;
>     }
> 
>     for (i = 0; i < ctx->nb_outputs; i++) {
>         avcodec_flush_buffers(movie->st[i].st->codec);
>         movie->st[i].done = 0;
>     }
>     movie->eof = 0;
>     return 0;
> }

Unrelated: I suspect this could be reused for the initial seek.


> /**
>  * Try to push a frame the requested output.

*to* the requested output.

>  *
>  * @return  1 if a frame was pushed on the requested output,
>  *          0 if another attempt is possible,
>  *          <0 AVERROR code
>  */
> static int movie_push_frame(AVFilterContext *ctx, unsigned out_id)
> {
>     MovieContext *movie = ctx->priv;
>     AVPacket *pkt = &movie->pkt;
>     MovieStream *st;
>     int ret, got_frame = 0, pkt_out_id;
>     AVFilterLink *outlink;
>     AVFilterBufferRef *buf;
> 

>     if (!movie->pkt.size) {
>         if (movie->eof) {
>             if (movie->st[out_id].done) {
>                 if (movie->loop_count != 1) {
>                     ret = rewind_file(ctx);
>                     if (ret < 0)
>                         return ret;
>                     movie->loop_count -= movie->loop_count > 1;
>                     av_log(ctx, AV_LOG_VERBOSE, "Stream finished, looping.\n");
>                     return 0; /* retry */
>                 }
>                 return AVERROR_EOF;
>             }
>             /* packet is already ready for flushing */
>         } else {
>             ret = av_read_frame(movie->format_ctx, &movie->pkt0);
>             if (ret < 0) {
>                 av_init_packet(&movie->pkt0); /* ready for flushing */
>                 *pkt = movie->pkt0;
>                 if (ret == AVERROR_EOF) {
>                     movie->eof = 1;
>                     return 0; /* start flushing */
>                 }
>                 return ret;
>             }
>             *pkt = movie->pkt0;
>         }
>     }
> 
>     pkt_out_id = pkt->stream_index > movie->max_stream_index ? -1 :
>                  movie->out_index[pkt->stream_index];
>     if (pkt_out_id < 0) {
>         av_free_packet(&movie->pkt0);
>         pkt->size = 0; /* ready for next run */
>         pkt->data = NULL;
>         return 0;
>     }
>     st = &movie->st[pkt_out_id];
>     outlink = ctx->outputs[pkt_out_id];
> 
>     switch (st->st->codec->codec_type) {
>     case AVMEDIA_TYPE_VIDEO:
>         ret = avcodec_decode_video2(st->st->codec, movie->frame, &got_frame, pkt);
>         break;
>     case AVMEDIA_TYPE_AUDIO:
>         ret = avcodec_decode_audio4(st->st->codec, movie->frame, &got_frame, pkt);
>         break;
>     default:
>         ret = AVERROR(ENOSYS);
>         break;
>     }
>     if (ret < 0) {
>         av_log(ctx, AV_LOG_WARNING, "Decode error: %s\n", av_err2str(ret));
>         return 0;
>     }

>     if (!ret)
>         ret = pkt->size;

why this?

> 
>     pkt->data += ret;
>     pkt->size -= ret;

>     if (movie->pkt.size <= 0) {

&movie->pkt == pkt?
In this case is less confusing to use just pkt->size consistently.

>         av_free_packet(&movie->pkt0);
>         pkt->size = 0; /* ready for next run */
>         pkt->data = NULL;
>     }
>     if (!got_frame) {
>         if (!ret)
>             st->done = 1;
>         return 0;
>     }
> 
[...]
-- 
FFmpeg = Fostering and Free Merciless Portable Elitarian Gigant


More information about the ffmpeg-devel mailing list