Re: [FFmpeg-devel] [PATCH] doc/examples/muxing: code rewrite with improved readability and fixed issues

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

Paolo Prete:
> Please review this. It's a code rewrite of doc/examples/muxing.c which improves readability and fixes issues.
> More specifically:
> *) Original functions of muxing.c have generic/unclear/ambiguous names and they don't group logically relatedblocks of code: this makes the code hard to read.See for example open_audio/video() and add_stream(), which mix initialization of the encoders' and muxers' stuff.A redundant structure with an ambiguous name ("struct OutputStream"), which is not part of API, is widely used too.The patch uses functions with clearer names and with blocks of code strictly logically related, such as: init_encoder(), init_avframe(), init_muxer(), convert_frame(), encode_frame(), mux_encoded_pkt()...

Using a structure for the user's data is actually intentional, because
users will probably use one, too. And given that it is the user's
structure it is of course not part of the public API.

> *) Errors are not always properly managed: in some cases they are not propagated to the main() function and a call to exit(1) is forced, causing memory leaks. This has been fixed as well.
> *) The program doesn't work for some extensions (try for example: m4v or mjpeg). In addition, it gives weird results when the filename doesn't have an extension.This has been fixed by checking the filename and supporting a fixed set of extensions.
> *) The program claims to output raw images by using '%%d' in the filename, but it actually doesn't. This has been fixed as well.
> *) Input parameters such as width, height, sample_rate and ch_layout are now exposed at the level of the main() function and they are much easier to customize.
> *) fprintf() has been replaced with av_log()
> 

This list alone implies that this patch must be split into small,
self-contained pieces that can be reviewed (whereas the current patch
can't).

There is just one thing that I immediately noticed:

> 
> +    AVCodecParameters audio_in_params = {
> +        .codec_type  = AVMEDIA_TYPE_AUDIO,
> +        .format      = AV_SAMPLE_FMT_S16,
> +        .sample_rate = 44100,
> +        .ch_layout   = (AVChannelLayout)AV_CHANNEL_LAYOUT_STEREO
> +    },
> +    video_in_params = {
> +        .codec_type = AVMEDIA_TYPE_VIDEO,
> +        .width      = 352,
> +        .height     = 288,
> +        .format     = AV_PIX_FMT_YUV420P
> +    },
> +    video_enc_params  = { 0 }, audio_enc_params = { 0 };

sizeof(AVCodecParameters) is not public, you must not put
AVCodecParameters on the stack.

- Andreas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".