[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