[FFmpeg-devel,v4,4/4] lavf: Add documentation for private "Context" classes

Doxygen thinks any text like "Context for foo" is a link to a struct called "Context".
Add a description and a better link, to avoid confusing readers.
---
 libavformat/async.c | 3 +++
 libavformat/cache.c | 3 +++
 2 files changed, 6 insertions(+)

On date Wednesday 2024-05-15 16:54:22 +0100, Andrew Sayers wrote:
> Doxygen thinks any text like "Context for foo" is a link to a struct called "Context".
> Add a description and a better link, to avoid confusing readers.
> ---
>  libavformat/async.c | 3 +++
>  libavformat/cache.c | 3 +++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/libavformat/async.c b/libavformat/async.c
> index e096b0bc6f..3c28d418ae 100644
> --- a/libavformat/async.c
> +++ b/libavformat/async.c
> @@ -53,6 +53,9 @@ typedef struct RingBuffer
>      int           read_pos;
>  } RingBuffer;
>  
> +/**
> + * @brief @ref md_doc_2context "Context" for testing async
> + */
>  typedef struct Context {
>      AVClass        *class;
>      URLContext     *inner;
> diff --git a/libavformat/cache.c b/libavformat/cache.c
> index 5f78adba9d..3cc0edec82 100644
> --- a/libavformat/cache.c
> +++ b/libavformat/cache.c
> @@ -52,6 +52,9 @@ typedef struct CacheEntry {
>      int size;
>  } CacheEntry;
>  
> +/**
> + * @brief @ref md_doc_2context "Context" for a cache
> + */
>  typedef struct Context {
>      AVClass *class;
>      int fd;

Not sure, these are private structs and we enforce no use of internal
markup for those, and we can assume internals developers are already
acquainted with contexts and such so this is probably adding no value.

On Wed, May 22, 2024 at 12:08:29PM +0200, Stefano Sabatini wrote:
> On date Wednesday 2024-05-15 16:54:22 +0100, Andrew Sayers wrote:
> > Doxygen thinks any text like "Context for foo" is a link to a struct called "Context".
> > Add a description and a better link, to avoid confusing readers.
> > ---
> >  libavformat/async.c | 3 +++
> >  libavformat/cache.c | 3 +++
> >  2 files changed, 6 insertions(+)
> > 
> > diff --git a/libavformat/async.c b/libavformat/async.c
> > index e096b0bc6f..3c28d418ae 100644
> > --- a/libavformat/async.c
> > +++ b/libavformat/async.c
> > @@ -53,6 +53,9 @@ typedef struct RingBuffer
> >      int           read_pos;
> >  } RingBuffer;
> >  
> > +/**
> > + * @brief @ref md_doc_2context "Context" for testing async
> > + */
> >  typedef struct Context {
> >      AVClass        *class;
> >      URLContext     *inner;
> > diff --git a/libavformat/cache.c b/libavformat/cache.c
> > index 5f78adba9d..3cc0edec82 100644
> > --- a/libavformat/cache.c
> > +++ b/libavformat/cache.c
> > @@ -52,6 +52,9 @@ typedef struct CacheEntry {
> >      int size;
> >  } CacheEntry;
> >  
> > +/**
> > + * @brief @ref md_doc_2context "Context" for a cache
> > + */
> >  typedef struct Context {
> >      AVClass *class;
> >      int fd;
> 
> Not sure, these are private structs and we enforce no use of internal
> markup for those, and we can assume internals developers are already
> acquainted with contexts and such so this is probably adding no value.

Ah, yeah the use case isn't obvious if you haven't tripped over it...

Imagine you're a new user trying to learn FFmpeg, and you find yourself at
https://ffmpeg.org/doxygen/trunk/structAVAudioFifo.html - the first word
appears to be a link for this "context" thing you've been hearing about[1],
so you drop what you're doing to investigate.  It links you to a struct that
looks promisingly general, but eventually turns out to be some random internal
struct with a misleading name.  Now you've wasted a bunch of time and forgotten
what you were doing.

The other patch fixes the examples I've found in the code, but doxygen links
all instances of the word "Context" to this struct, confusing newbies.  This
patch ensures that when future developers say "Context" in a comment, the page
doxygen links them to will point them in the right direction.

I had previously assumed it was a deliberate decision to include private files
in the documentation, so I didn't look that carefully into workarounds that
would break links to these private structures.  But you've implied elsewhere
that's not the case, so is it worth looking into solutions that made "Context"
link to the context document, at the cost of making it impossible to link to
these private structs at all?

[1] to be clear, this is the current behaviour of the page on the live site

Andrew Sayers:
> Doxygen thinks any text like "Context for foo" is a link to a struct called "Context".
> Add a description and a better link, to avoid confusing readers.
> ---
>  libavformat/async.c | 3 +++
>  libavformat/cache.c | 3 +++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/libavformat/async.c b/libavformat/async.c
> index e096b0bc6f..3c28d418ae 100644
> --- a/libavformat/async.c
> +++ b/libavformat/async.c
> @@ -53,6 +53,9 @@ typedef struct RingBuffer
>      int           read_pos;
>  } RingBuffer;
>  
> +/**
> + * @brief @ref md_doc_2context "Context" for testing async
> + */
>  typedef struct Context {
>      AVClass        *class;
>      URLContext     *inner;
> diff --git a/libavformat/cache.c b/libavformat/cache.c
> index 5f78adba9d..3cc0edec82 100644
> --- a/libavformat/cache.c
> +++ b/libavformat/cache.c
> @@ -52,6 +52,9 @@ typedef struct CacheEntry {
>      int size;
>  } CacheEntry;
>  
> +/**
> + * @brief @ref md_doc_2context "Context" for a cache
> + */
>  typedef struct Context {
>      AVClass *class;
>      int fd;

These structures should be renamed instead of adding these comments
(which are pointless for internal developers). I just sent a patch for that.
Thanks for pointing out the issue.

- Andreas

On Wed, May 22, 2024 at 05:24:36PM +0200, Andreas Rheinhardt wrote:
> These structures should be renamed instead of adding these comments
> (which are pointless for internal developers). I just sent a patch for that.
> Thanks for pointing out the issue.

Oh, great!  So the next version of this patchset will skip this patch,
and will reduce links like this:

> + * @brief @ref md_doc_2context "Context" for a cache

down to:

> + * @brief @ref Context for a cache

I don't see a way of removing the "@ref" without doing something nasty,
like making a fake "Context" struct and shoving the documentation in there.

Also, if someone does something strange like this:

> + * @brief @ref Context "structure" (actually an enum)

doxygen won't render it the way the author expects.  I don't expect that to
happen much in the real world though.