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

[Paolo Prete]

Sorry: I had problems with my email client in formatting the previous message. I just try to resend it.

>> Il sabato 18 giugno 2022, 17:18:18 CEST, Leo Izen <leo.izen at gmail.com> ha scritto:
>>On 6/18/22 08:06, Paolo Prete wrote:
>> +{
>> +    if (num)
>> +        av_log(NULL, AV_LOG_ERROR, "%s (error '%s')\n", s, av_err2str(*num));
>> +    else
>> +        av_log(NULL, AV_LOG_ERROR, "%s\n", s);
> +}
>This does not need to be a pointer. Convention is that negative values
>are errors and nonnegative values are not. So you could always use
>something like: if (num < 0).

The pointer makes clearer, when I call the function, that I'm not managing an error with a number id.

>> -    AVPacket *tmp_pkt;
>> +    if (type == AVMEDIA_TYPE_AUDIO)
>> +        enc_time_base = ((AVRational *)out_fmt_ctx->opaque)[0];
>> +    else
>> +        enc_time_base = ((AVRational *)out_fmt_ctx->opaque)[1];
>>
>Why are you referencing the opaque elements of out_fmt_ctx?

I pass, through the opaque element, user data to the muxer. That data (the timebases of the audio and video encoders) will be used for rescaling ts to the
muxer timebases. Otherwise I would have to pass them through the function's params, which would make the function prototype longer and less readable.

>>
>> -static int write_frame(AVFormatContext *fmt_ctx, AVCodecContext *c,
>> -                      AVStream *st, AVFrame *frame, AVPacket *pkt)
>> +static int is_extension_supported(const char *filename)
>Why are you artificially limiting what is permitted?

>From what I see, not all extensions are supported without modifying the default settings. In fact, as specified in the commit msg, some extensions don't work and the original muxing.c fails with them.

>> +    if (!(c = avcodec_find_encoder(id))) {
>> +        avformat_free_context(tmp_fctx);
>> +        return ret;
>You probably don't want to return "ret" here as you don't assign it.

ret is assigned to 0 (= success) at the definition of the variable. And in this case, the function succeeds: the returned AVCodecParameter is set with AV_CODEC_ID_NONE

>> -    if (oc->oformat->flags & AVFMT_GLOBALHEADER)
>> -        c->flags |= AV_CODEC_FLAG_GLOBAL_HEADER;
>> +    return ret;
> Again, why are you returning ret if you are not assigning to it?

as above, ret is assigned to 0.

>> +    codec = avcodec_find_encoder(params->codec_id);
>> +    *enc_ctx = avcodec_alloc_context3(codec);
>Don't attempt to allocate anything until after you check if the codec is
>found.

Is it really necessary?
It has been already checked in the line with "if (process_audio/video) {" and the API doxy says that a NULL parameter will only cause that codec-specific defaults won't be initialized (so  it appears safe to me).

>> +    if (!codec) {
>> +        log_error("Could not allocate the encoding context", NULL);
>This error message does not match the check, which is if the codec is found.
>> +        return AVERROR_EXIT;
>return AVERROR_CODEC_NOT_FOUND;

Instead of changing the error msg, I think that what was wrong is the check. I fixed it with
if (!(*enc_ctx = avcodec_alloc_context3(codec))) {

>> +        (*enc_ctx)->sample_rate = params->sample_rate;
>> +        (*enc_ctx)->time_base  = (AVRational){1, params->sample_rate};
>Use av_make_q to avoid casting.

Done, thanks.

> +        return ret;
> +    } else
> +        return 0;This violates the coding style, you need to use braces {} for the else
block if you also use it for the if block.

Done, thanks.

> +    if ((ret = av_frame_get_buffer(*frame, 0)) < 0) {
> +        log_error("Could not allocate buffer for AVFrame", &ret);
> +        return AVERROR(ENOMEM);
> +    } else
> +        return 0;
You don't need the else block here at all.

Done, thanks.

>> +static int init_video_convert(struct SwsContext **ctx, AVCodecParameters *in_params,
>> +                              AVCodecParameters *out_params)
>> +{
>This paper-thin wrapper function is unnecessary, just inline it.

This is intentional: even if it's a paper-thin wrapper, it shortens the code of the main() function
by making it quicker to understand that the audio/video converters are initialized with their respective AVCodecParameters. I would wait for feedback about this from other readers, before changing the code.

> +
> +    if (!tincr) {
> +        t      = 0;
> +        tincr  = 2 * M_PI * 110.0 / frame->sample_rate;
> +        /* increment frequency by 110 Hz per second */
> +        tincr2  = tincr / frame->sample_rate;
> What are you doing here? Why are you doing it?
>>      }
>> -
>> -    return write_frame(oc, c, ost->st, frame, ost->tmp_pkt);
>> +    for (j = 0; j <frame->nb_samples; j++) {
>> +        v = (int)(sin(t) * 10000);
>> +        for (i = 0; i < frame->ch_layout.nb_channels; i++)
>> +            *data++ = v;
>> +        t    += tincr;
>> +        tincr += tincr2;
>> +    }
>> +    frame->pts = frame->nb_samples*(++frame_ctr);
>If you're trying to populate a stream, you should be using the aevalsrc
>filter, which exists for exactly this purpose. Otherwise just populate
>it with zeroes (silence).
>>  }
>>
>> -/**************************************************************/
>> -/* video output */
>> -
>> -static AVFrame *alloc_picture(enum AVPixelFormat pix_fmt, int width, int height)
>> +static void fill_dummy_yuv420p_frame(AVFrame *frame)
>>  {
>There's a testsrc filter, or just fill a frame with zeroes (black).
>Don't reinvent the wheel in an example, that discourages people from
>using features that exist.

This is all copied from the original muxing.c example. These dummy audio/video frames consist in few lines of code and they are common in doc/example files. See also encode-audio.c, encode-video.c. Adding a filtering context to the current example would consequently require to patch (and maybe rename) the other files as well. And if you patch in that way, for example, "encode-audio.c" the reader won't focus on the encoding task, because the filtering block of code would be somewhat distracting.

>> -            }
>> +    if (ret < 0) {
>You need to check for AVERROR(EAGAIN).
>> +        av_log(NULL, AV_LOG_ERROR,
>> +              "Error sending frame to the encoder (error '%s')\n", av_err2str(ret));
>> +        return ret;

Is it really necessary to check AVERROR(EAGAIN) when sending the frame to the encoder, in this specific case?
The function is written in a way that the encoder's output is always read before sending new frames. Note that in the original muxing.c this is not checked as well

>> -    return write_frame(oc, ost->enc, ost->st, get_video_frame(ost), ost->tmp_pkt);
>> +    enum AVMediaType *type = (enum AVMediaType *)(fr->opaque);
>Why are you reading from the opaque structure of the frame. Are you sure
>this is what you wanted to do?

Yes, it just stores an additional info for the frame (it says if the frame contains video or audio data), which will be used later in "frame_exceeds_stream_duration()" function.

>>                "The output format is automatically guessed according to the file extension.\n"
>> -              "Raw images can also be output by using '%%d' in the filename.\n"
>> +              "BMP or JPEG images can also be output by using '%%d' in the filename.\n"
>>                "\n", argv[0]);
>> -        return 1;
>> +        return AVERROR_EXIT;
>This return value is sent to the operating system with the exit() system
>call so you don't actually want to return an AVERROR value here.

I replaced it with return 1.
I also allocated AVCodecParameter with the proper alloc() function, as Andreas suggested.
A new patch is attached to this mail.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-doc-examples-muxing-code-rewrite-with-improved-reada.patch
Type: text/x-patch
Size: 40711 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20220619/f571f0b2/attachment.bin>