[FFmpeg-devel] Would like to work on lavf-based concatenation tool for GSoC qualification

Geza Kovacs gkovacs
Thu Apr 2 11:09:16 CEST 2009


On Wed, 2009-04-01 at 13:50 -0700, Baptiste Coudurier wrote:
> Yes, please enhance ffmpeg binary. You can put code in a separate file,
> but the goal is to enhance ffplay and ffmpeg binaries, so factorizing
> the code is likely needed.
> 

I've attached a patch that adds said functionality as an ffmpeg
command-line option. I assume there's a more clean way to do this than
by introducing a conditional at the beginning of av_encode(); I assume I
would probably want to modify opt_format and opt_output_file in the
final version instead, but for the sake of time this seemed to work best
with the code I had already written for now.

> General comment please try to keep lines reasonably short, I personally
> do not enforce 80 columns, but some people around here like it.
> 
> >> ------------------------------------------------------------------------
> >>
> >> #include <libavcodec/avcodec.h>
> >> #include <libavformat/avformat.h>
> >> #include <libavutil/avstring.h>
> >> #include <malloc.h>
> 
> Use av_malloc/etc.., see "libavutil/mem.h"

I've switched to av_malloc and have removed the superfluous casts.

> 
> >> /**
> >>  * open_input_files returns AVFormatContexts for a list of input files
> >>  * @param char **input_files list of input files,
> >>  * @param int num_input_files number of input files.
> >>  * @return returns NULL on error, otherwise a list of input format contexts
> >>  */
> >> AVFormatContext** open_input_files(char **input_files, int num_input_files) {
> >>     AVFormatContext **input_format_contexts = (AVFormatContext**)malloc(num_input_files*sizeof(AVFormatContext*));
> 
> Useless cast in plain C, there are plenty in the file.
> 
> >>     AVFormatParameters **input_format_parameters = (AVFormatParameters**)malloc(num_input_files*sizeof(AVFormatParameters*));
> >>     int i;
> >>     for (i = 0; i < num_input_files; ++i) {
> >>         if (av_open_input_file(input_format_contexts+i, input_files[i], NULL, 0, NULL) != 0) {
> 
> < 0 is usually used here.
> 
> >>             fprintf(stderr, "Error while opening %s\n", input_files[i]);
> >>             fflush(stderr);
> 
> Do you really need to flush here ?
> 

I've removed the superfluous flushes.

> >> [...]
> >>
> >> /**
> >>  * close_input_files closes a list of AVFormatContext*
> >>  * @param AVFormatContext **input_format_contexts list of input format contexts
> >>  * @param int num_input_files number of input files.
> >>  * @return returns nothing
> >>  */
> >> void close_input_files(AVFormatContext **input_format_contexts, int num_input_files) {
> >>     int i;
> >>     for (i = 0; i < num_input_files; ++i) {
> >>         /* av_close_input_stream(input_format_contexts[i]); */
> >>         av_close_input_file(input_format_contexts[i]);
> >>         /* av_free(input_format_contexts[i]); */
> >>     }
> 
> Please remove debugging code.

I've removed all commented-out code.

> 
> >> [...]
> >>
> >>             if (!color_table_id_set) {
> >>                 color_table_id_set = 1;
> >>                 color_table_id = input_format_contexts[i]->streams[j]->codec->color_table_id;
> >>             }
> 
> There is a FIXME near color_table_id, did you encounter a case where you
> need to check this ?

I didn't; that was merely for safety. I've removed that check.

> 
> The mechanism you are using will be good for stream copying.
> There are however some check lacking:
> 
> "extradata" handling: extradata might differ from one stream to another,
> this happens offently with H.264
> 

I assume all I need to do in this case is copy over extradata and
extradata_size from the input stream's AVCodecContext to the output
stream as I did in the patch, correct?

> The code must be ready to add "filters" if video resolution does not
> match, or audio sample format differs. This will be needed.
> 

I aim to introduce the filters by splitting what is currently
"check_same_settings" into multiple functions each performing a check
and adding a filter if necessary, rather than emitting an error on
mismatch.



More information about the ffmpeg-devel mailing list