[FFmpeg-devel] [PATCH v4 1/4] doc: Explain what "context" means

[Stefano Sabatini]

On date Wednesday 2024-05-22 17:07:51 +0100, Andrew Sayers wrote:
[...]
> > > diff --git a/doc/context.md b/doc/context.md
> > > new file mode 100644
> > > index 0000000000..fb85b3f366
> > > --- /dev/null
> > > +++ b/doc/context.md
> > > @@ -0,0 +1,394 @@
> > > +# Introduction to contexts
> > > +
> > > +“%Context”
> > 
> > Is this style of quoting needed? Especially I'd avoid special markup
> > to simplify unredendered text reading (which is the point of markdown
> > afterall).
> 
> Short answer: I'll change it in the next patch and see what happens.
> 
> Long answer: HTML quotes are ugly for everyone, UTF-8 is great until someone
> turns up complaining we broke their Latin-1 workflow.  I've always preferred
> ASCII-only representations for that reason, but happy to try the other way
> and see if anyone still cares.

I think it's safe to assume UTF-8 as the default nowadays, and every
other encoding broken in same way. Latin-1 is assumed probably only by
a minor group of FFmpeg users.
 
> > 
> > > is a name for a widely-used programming idiom.
> > 
> > > +This document explains the general idiom and the conventions FFmpeg has built around it.
> > > +
> > > +This document uses object-oriented analogies to help readers familiar with
> > > +[object-oriented programming](https://en.wikipedia.org/wiki/Object-oriented_programming)
> > > +learn about contexts.  But contexts can also be used outside of OOP,
> > > +and even in situations where OOP isn't helpful.  So these analogies
> > > +should only be used as a first step towards understanding contexts.
> > > +
> > > +## “Context” as a way to think about code
> > > +
> > > +A context is any data structure that is passed to several functions
> > > +(or several instances of the same function) that all operate on the same entity.
> > > +For example, [object-oriented programming](https://en.wikipedia.org/wiki/Object-oriented_programming)
> > > +languages usually provide member functions with a `this` or `self` value:
> > > +
> > 
> > > +```c
> > > +class my_cxx_class {
> > > +  void my_member_function() {
> > > +    // the implicit object parameter provides context for the member function:
> > > +    std::cout << this;
> > > +  }
> > > +};
> > > +```
> > 
> > I'm not convinced this is really useful: if you know C++ this is
> > redundant, if you don't this is confusing and don't add much information.
> 
> The example is there to break up a wall of text (syntax-highlighted in the
> rendered output), and to let the reader know that this is going to be one of
> those documents that alternates between text and code, so they're ready for the
> more substantive examples later on.  I take the point about C++ though -
> would this Python example be more readable?
> 
>     class MyClass:
>         def my_func(self):
>             # If a Python function is part of a class,
>             # its first parameter must be an instance of that class
> 

But again, this is redundnat if you know Python, otherwise is not very
helpful if you don't. In this case probably it's good just to keep the
mention to self/this, without a specific example.

> > 
> > > +
> > > +Contexts are a fundamental building block of OOP, but can also be used in procedural code.
> > 
> > I'd drop this line, and drop the anchor on OOP at the same time since
> > it's adding no much information.
> 
> Fundamentally, this document addresses two audiences:
> 
> 1. people coming from a non-OOP background, who want to learn contexts
>    from first principles, and at best see OOP stuff as background information
> 
> 2. people coming from an OOP background.  There's no polite way to say this -
>    their incentive is to write FFmpeg off as a failed attempt at OOP, so they
>    don't have to learn a new way of working that's just different enough to
>    make them feel dumb
> 
> I think a good way to evaluate the document might be to read it through twice,
> stopping after each paragraph to ask two unfair questions...
> 
> 1. what has this told me about FFmpeg itself, as opposed to some other thing
>    you wish I cared about?
> 
> 2. couldn't you have just done this the standard OOP way?
> 
> The earlier paragraph acknowledged that contexts resemble OOP (telling the OOP
> audience we get it), then this paragraph adds "but they're not the same"
> (telling the OOP audience we disagree).  To be more useful to non-OOP folk,
> how about:
> 

>     Contexts can be a fundamental building block of OOP, but can also be used in
>     procedural projects like FFmpeg.

What perplexes me is that "context" is not part of the standard OOP
jargon, so this is probably adding more to the confusion. I think the
target here is to clarify what context is meant for in FFmpeg, and
this can be simply defined without any reference to OOP:

|A context is a structure used to contain data (allocated memory,
|configuration, and/or state) for operations working on the same
|entity.

As such a context is just a glorified struct referecenced by a set of
functions operating on them, with the convention (common in most C
APIs) that this is the first argument of these functions.

Then we have AVClass+log+options as a trick to define common
functionality for the most complex "contexts".

This is also why I'm concerned with the idea of explainig this with a
strong focus on OOP: the effect is that the reader not familiar with
OOP is convinced that she needs to be familiar with OOP to understand
it (which is not the case). That's also why I'm in favor of having a
comparison with OOP in a dedicated section: this might help the
OOP-minded reader, and might be skipped by the other ones.

[...]
> > I'm not very convinced by the callback example. The use of contexts in
> > the FFmpeg API is very much simpler, it is used to keep track of
> > configuration and state (that is they track the "object" where to
> > operate on), so the callback example here is a bit misleading.
> > 
> > Callbacks are used in the internals to implement different elements
> > (codecs, protocols, filters, etc...) implementing a common API, but in
> > this case the relation with "contexts" is less straightforward.
> 
> I go back and forth on this one, but your point made me think about it
> in a new way...
> 

> AVIOContext::read_packet is a callback function, and a reader who has just
> learnt about contexts would naturally assume we intend its first argument
> to be interpreted as a context.  Given that new readers are likely to learn
> avio_alloc_context() around the same time as reading this document,
> it's important we give them the tools to understand that function.
> 
> How about changing the topmost callback example to read data from a FILE*
> (without mentioning AVIOContext), then emphasising how you can think of it
> as a context despite not following FFmpeg's rules, then finally mentioning
> how you could pass the callback to avio_alloc_context() if you wanted?

Let's discuss this in the context of the new patch.

> > > + at warning Guaranteeing to use contexts does not mean guaranteeing to use
> > > +object-oriented programming.  For example, FFmpeg creates its contexts
> > > +procedurally instead of with constructors.
> > 
> > I'm afraid this is more confusing than helpful, since the FFmpeg API
> > is not OOP. I'd drop this sentence.
> 
> My concern is that if an OOP reader asks "couldn't you have just done this
> the standard OOP way?", they will be tempted to answer "oh, so you *used to*
> fail at OOP but nowadays you promise to do it right", and not bother reading
> any further.  So there needs to be something eye-catching here, but yes this
> paragraph needs to be more useful to non-OOP readers.

If you write:
> FFmpeg creates its contexts procedurally instead of with constructors.

then I don't know what this means, since there are no explicit
"contructors" in plain C as they are not part of the language
(although you can mimic constructors using special conventions, but
this is something built on top of the language).

>[...]
> This will probably need to be rewritten based on the callback discussion,
> so I'll think about ways to change this at the same time.
> > > +Curl's struct is declared as `struct <type> { ... }`, whereas FFmpeg uses
> > > +`typedef struct <type> { ... } <type>`.  These conventions are used with both
> > > +context and non-context structs, so don't say anything about contexts as such.
> > > +Specifically, FFmpeg's convention is a workaround for an issue with C grammar:
> > > +
> > > +```c
> > > +void my_function( ... ) {
> > > +  int                my_var;        // good
> > > +  MD5_context        my_curl_ctx;   // error: C needs you to explicitly say "struct"
> > > +  struct MD5_context my_curl_ctx;   // good: added "struct"
> > > +  AVMD5              my_ffmpeg_ctx; // good: typedef's avoid the need for "struct"
> > > +}
> > > +```
> > > +
> > > +Both MD5 implementations are long-tested, widely-used examples of contexts
> > > +in the real world.  They show how contexts can solve the same problem
> > > +in different ways.
> > 
> > I'm concerned that this is adding more information than really
> > needed. Especially comparing with internals of curl means that now the
> > docs needs to be kept in synch also with the curl's API, meaning that
> > it will be outdated very soon. I'd rather drop the curl comparison
> > altogether.
> 
> The value of this section depends on the reader...
> 
> This tells a non-OOP reader that FFmpeg didn't invent contexts, and reasonable
> people can disagree about what they mean.  So when we get into the ambiguous
> cases later on, they have a better idea about which things are just how
> contexts work, and which are specifically how FFmpeg uses them.
> 
> This tells an OOP reader that it's not "OOP standard, FFmpeg non-standard",
> it's that FFmpeg is using a C standard that's not used in OOP languages.
> 
> Curl's `MD5_context` was last modified in 2020 (interestingly, to get rid of
> the `typedef struct` trick).  It's a slow-moving target, but you're right it's
> not a static one.
> 
> I'd argue the above means this should be *somewhere* in the document, but not
> necessarily here in the middle.  I'll see if it works better as a paragraph
> here and an appendix or something at the bottom.

Yes, moving to a less prominent section might help, since we want to
avoid the long-wall-of-text effect with information not impacting the
basic understanding.

[...]
> > > +To support a specific codec, AVCodecContext's private context is set to
> > > +an encoder-specific data type.  For example, the video codec
> > > +[H.264](https://en.wikipedia.org/wiki/Advanced_Video_Coding) is supported via
> > > +[the x264 library](https://www.videolan.org/developers/x264.html), and
> > > +implemented in X264Context.
> > 
> > > Although included in the documentation, X264Context is not part of the public API.
> > 
> > Why included in the doc? That is a private struct and therefore should
> > not be included in the doxy.
> 

> Doxygen doesn't provide an obvious mechanism to include only the public API.
> Changing that would be at best a big job, and it isn't obvious where to even
> start until/unless e.g. [1] gets merged in.  It seems like a better plan
> to put the warning in and take it out if and when the site gets updated.

I think it's fine to keep as is, this is somehow a doxygen glitch, but
at the same time might help with internal development (in this case
you want to know the internal structures).