Re: [FFmpeg-devel] [PATCH v3 1/3] doc: Explain what "context" means

From: Stefano Sabatini <stefasab@gmail.com>
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH v3 1/3] doc: Explain what "context" means
Date: Wed, 22 May 2024 12:37:37 +0200
Message-ID: <Zk3K8WEj9o/ZZH4n@mariano> (raw)
In-Reply-To: <Zjf0ZK2No4ir51iq@andrews-2024-laptop.sayers>

On date Sunday 2024-05-05 22:04:36 +0100, Andrew Sayers wrote:
> I'm still travelling, so the following thoughts might be a bit
> half-formed.  But I wanted to get some feedback before sitting down
> for a proper think.
[...]
> > > I've also gone through the code looking for edge cases we haven't covered.
> > > Here are some questions trying to prompt an "oh yeah I forgot to mention
> > > that"-type answer.  Anything where the answer is more like "that should
> > > probably be rewritten to be clearer", let me know and I'll avoid confusing
> > > newbies with it.
> > > 
> > 
> > > av_ambient_viewing_environment_create_side_data() takes an AVFrame as its
> > > first argument, and returns a new AVAmbientViewingEnvironment.  What is the
> > > context object for that function - AVFrame or AVAmbientViewingEnvironment?
> > 
> > But this should be clear from the doxy:
> > 
> > /**
> >  * Allocate and add an AVAmbientViewingEnvironment structure to an existing
> >  * AVFrame as side data.
> >  *
> >  * @return the newly allocated struct, or NULL on failure
> >  */
> > AVAmbientViewingEnvironment *av_ambient_viewing_environment_create_side_data(AVFrame *frame);
> 
> I'm afraid it's not clear, at least to me.  I think you're saying the
> AVFrame is the context because a "create" function can't have a
> context any more than a C++ "new" can be called as a member.  But the
> function's prefix points to the other conclusion, and neither signal
> is clear enough on its own.

No, what I'm saying is that in some cases you don't need to think in
terms of contexts, in this case there is no context at all, the
function takes a frame and modify it, and returns the ambient
environment to be used by the following functions. This should be very
clear by reading the doxy. There is no rule dictating the first param
of each FFmpeg function should be a "context".

> 
> My current thinking is to propose separate patches renaming arguments
> to `ctx` whenever I find functions I can't parse.  That's not as good
> as a simple rule like "the first argument is always the context", but
> better than adding a paragraph or two about how to read the docs.

There cannot be such rule, because it would be false in many cases.

> > Also, you are assuming that all the function should have a
> > context. That's not the case, as you don't always need to keep track
> > of a "context" when performing operations.
> 
[...]
> > > av_channel_description_bprint() takes a `struct AVBPrint *` as its first
> > > argument, then `enum AVChannel`.  Is the context AVBPrint, AVChannel,
> > > or both?  Does it make sense for a function to have two contexts?
> > 
> > Again, this should be clear from the doxy:
> > /**
> >  * Get a human readable string describing a given channel.
> >  *
> >  * @param buf pre-allocated buffer where to put the generated string
> >  * @param buf_size size in bytes of the buffer.
> >  * @param channel the AVChannel whose description to get
> >  * @return amount of bytes needed to hold the output string, or a negative AVERROR
> >  *         on failure. If the returned value is bigger than buf_size, then the
> >  *         string was truncated.
> >  */
> > int av_channel_description(char *buf, size_t buf_size, enum AVChannel channel);
> > 
> > /**
> >  * bprint variant of av_channel_description().
> >  *
> >  * @note the string will be appended to the bprint buffer.
> >  */
> > void av_channel_description_bprint(struct AVBPrint *bp, enum AVChannel channel_id);
> 

> I think you're saying that I should look at which word appears more
> often in the doxy ("channel") rather than which word appears first in
> the argument list ("buf")?  As above, the solution might be to rename
> the variable in a separate patch rather than teach people another
> special case.

This is more about the semantics described in English language by the
doxy (which is normative). Again, thinking in terms of "contexts" is
misleading in this case.

In this case you have two functions, av_channel_description writing a
string to a buffer with fixed size, the second modifying an "AVBPrint"
struct, which is a high-level buffer providing more flexibility (and
"bprint" is used as a verb in the doxy, which might be misleading).

Note that both signatures are mimicing the standard C library
convention:
memcpy(dst, dst_size, src)

which in turn is a mnemonics for:
dst = src

meaning that we are copying data from src to dst.

You might think that in fact you are operating on a context (the dst
buffer or the AVBPrint struct), but you don't need to introduce the
concept of context for these simple functions.
_______________________________________________
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".