Re: [FFmpeg-devel] [PATCH] New API usage example (reading, converting, encoding and muxing an audio file)

[Paolo Prete <p4olo_prete-at-yahoo.it@ffmpeg.org>]

 

    Il martedì 7 giugno 2022, 13:56:37 CEST, Anton Khirnov <anton@khirnov.net> ha scritto:  
 
 >Quoting Paolo Prete (2022-06-07 12:59:05)
>> What you say is true, IMHO, as long as the functions (in which the
>> code is split) do really group logically related tasks and they have
>> names that summarize what they are doing. In the examined case this is
>> not true, see for example (in muxing.c): static void
>> open_video(AVFormatContext *oc, const AVCodec *codec, OutputStream
>> *ost, AVDictionary *opt_arg)
>> 
>> The name "open_video" is too generic and it doesn't let the user know
>> what the function is actually doing, without jumping from line to line
>> in the code. In fact the function mixes different tasks: it opens the
>> codec, it allocates a frame, it copies the stream parameters to the
>> muxer.

>All the things it does relate to preparing video encoding. You might
>argue that the function name is suboptimal, in which case it should be
>improved. But it is certainly not true that the function just groups
>random unrelated code.

Not true. There's a step, inside the function, that does _not_ relate to preparing video encoding, then it should not be grouped into the same logical unit: 
    /* copy the stream parameters to the muxer */    ret = avcodec_parameters_from_context(ost->st->codecpar, c);
Then: how would you call the function? Obviously, "prepare_video_encoding()" would not be appropriate.

>> Same thing for write_audio_frame(), and in fact a comment is
>> put just above the function, and it says: "encode one audio frame and
>> send it to the muxer,,," ...which is obscure from the function's name
>> (and, again, the user is forced to jump often from a chunk to another
>> chunk of code in order to understand what the code is _generally_
>> doing).Note too that this can't be fixed by using more explicative or
>> longer names, because the functions mixes tasks which are _different_.
>> Therefore, these functions in many cases do not improve readability
>> and IMHO is better to have a longer code instead of forcing grouping
>> different tasks in the same function with an ambiguous name.

>Your argument seems to amount to "the existing structure is imperfect,
>so it is better to have no structure at all", 
Avoiding to split code, when not so useful, doesn't mean to "have no structure at all".The code I pasted has its own precise structure. 
with which I disagree.
>And even if you convinced me, adding a whole new example while keeping
>the old one is not a good solution - people would just be confused by
>multiple examples doing the same thing in different ways.
The new example doesn't do the same thing in different ways. In fact:
1) It reads from file, with customizable params (sample rate, sample fmt, channels ...) and not from a dummy generated audio. 2) It uses a custom I/O callback for accessing muxed data3) it operates on audio only.
You could argue that some tasks are already inside muxing.c, but this applies to encode_audio.c too. And having encode_audio.c + muxing.c doesn't confuse people IMHO.


  
_______________________________________________
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".