Re: [FFmpeg-devel] [PATCH 1/2] avformat: harmonise "- {decoding, encoding, demuxing, muxing}: " comments

[Andrew Sayers <ffmpeg-devel@pileofstuff.org>]

Hey, no problem :)

On 09/03/2024 16:06, Stefano Sabatini wrote:
> Hi, sorry for the slow reply.
> 
> On date Thursday 2024-02-29 16:23:06 +0000, Andrew Sayers wrote:
> 
>> There seems to be a convention for documenting ownership:
> 
> I find "ownership" a bit confusing, probably it's better to talk about
> context usage.

On reflection, it might be better to borrow some terms from function 
documentation.  So s/ownership/direction/ and s/meaning/description/

Here's a silly example of a well-documented function and equivalent 
struct using the proposed layout:

/**
  *
  * @brief AI-aging magic (functional version)
  *
  * @param[in,out] age   AI-generated age value
  *
  * - encoding: in
  * - decoding: out
  *
  * *Video encoding*
  *
  * Run all faces through a filter that makes them look
  * this many years old.
  *
  * Values outside of the range 1-100 will be ignored.
  *
  * *Audio decoding*
  *
  * Indicates the age of the primary speaker in years.
  * May be less accurate for non-English speakers.
  *
  * Values <0 indicate the age could not be determined.
  */
void example_function( ExampleContext* ctx, int age );

/**
  * @brief AI-aging magic (structural version)
  */
struct ExampleContext {

     /**
      *
      * @brief AI-generated age value
      *
      * - encoding: set by user
      * - decoding: set by libav
      *
      * *Video encoding*
      *
      * Run all faces through a filter that makes them look
      * this many years old.
      *
      * Values outside of the range 1-100 will be ignored.
      *
      * *Audio decoding*
      *
      * Indicates the age of the primary speaker in years.
      * May be less accurate for non-English speakers.
      *
      * Values <0 indicate the age could not be determined.
      */
     int age;

};

As a reader, I want to know how I'm supposed to engage with a variable 
(direction) before I can understand the deeper semantics (description). 
Representing the former with a bulleted list makes it easier to glance 
through the docs and find the variable I'm looking for, while 
representing the latter with headings and paragraphs makes it easier to 
focus on the bit I care about right now.

> 
>>
>>      /**
>>       * - encoding: (who sets this in encoding context)
>>       * - decoding: (who sets this in decoding context)
>>       */
>>      int foo;
> 
>>
>> Ensure all such comments start with a "-" and use lower case,
>> so doxygen formats them as a bulleted lists instead of one
>> long paragraph.
>>
>> Signed-off-by: Andrew Sayers <ffmpeg-devel@pileofstuff.org>
>> ---
>>   libavformat/avformat.h | 67 +++++++++++++++++++++---------------------
>>   1 file changed, 34 insertions(+), 33 deletions(-)
>>
>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>> index f4506f4cf1..35b7f78ec7 100644
>> --- a/libavformat/avformat.h
>> +++ b/libavformat/avformat.h
>> @@ -849,8 +849,8 @@ typedef struct AVStream {
>>       int index;    /**< stream index in AVFormatContext */
>>       /**
>>        * Format-specific stream ID.
>> -     * decoding: set by libavformat
>> -     * encoding: set by the user, replaced by libavformat if left unset
>> +     * - decoding: set by libavformat
>> +     * - encoding: set by the user, replaced by libavformat if left unset
>>        */
>>       int id;
>>   
>> @@ -871,13 +871,13 @@ typedef struct AVStream {
>>        * This is the fundamental unit of time (in seconds) in terms
>>        * of which frame timestamps are represented.
>>        *
>> -     * decoding: set by libavformat
>> -     * encoding: May be set by the caller before avformat_write_header() to
>> -     *           provide a hint to the muxer about the desired timebase. In
>> -     *           avformat_write_header(), the muxer will overwrite this field
>> -     *           with the timebase that will actually be used for the timestamps
>> -     *           written into the file (which may or may not be related to the
>> -     *           user-provided one, depending on the format).
>> +     * - decoding: set by libavformat
>> +     * - encoding: May be set by the caller before avformat_write_header() to
>> +     *             provide a hint to the muxer about the desired timebase. In
>> +     *             avformat_write_header(), the muxer will overwrite this field
>> +     *             with the timebase that will actually be used for the timestamps
>> +     *             written into the file (which may or may not be related to the
>> +     *             user-provided one, depending on the format).
>>        */
>>       AVRational time_base;
>>   
>> @@ -896,8 +896,9 @@ typedef struct AVStream {
>>        * If a source file does not specify a duration, but does specify
>>        * a bitrate, this value will be estimated from bitrate and file size.
>>        *
>> -     * Encoding: May be set by the caller before avformat_write_header() to
>> -     * provide a hint to the muxer about the estimated duration.
>> +     * - decoding: set by libavformat
>> +     * - encoding: may be set by the caller before avformat_write_header() to
>> +     *             provide a hint to the muxer about the estimated duration.
>>        */
>>       int64_t duration;
>>   
>> @@ -935,8 +936,8 @@ typedef struct AVStream {
>>        * For streams with AV_DISPOSITION_ATTACHED_PIC disposition, this packet
>>        * will contain the attached picture.
>>        *
>> -     * decoding: set by libavformat, must not be modified by the caller.
>> -     * encoding: unused
>> +     * - decoding: set by libavformat, must not be modified by the caller.
>> +     * - encoding: unused
>>        */
>>       AVPacket attached_pic;
>>   
>> @@ -1203,16 +1204,16 @@ typedef struct AVStreamGroup {
>>       /**
>>        * Group type-specific group ID.
>>        *
>> -     * decoding: set by libavformat
>> -     * encoding: may set by the user
>> +     * - decoding: set by libavformat
>> +     * - encoding: may set by the user
>>        */
>>       int64_t id;
>>   
>>       /**
>>        * Group type
>>        *
>> -     * decoding: set by libavformat on group creation
>> -     * encoding: set by avformat_stream_group_create()
>> +     * - decoding: set by libavformat on group creation
>> +     * - encoding: set by avformat_stream_group_create()
>>        */
>>       enum AVStreamGroupParamsType type;
>>   
>> @@ -1534,19 +1535,19 @@ typedef struct AVFormatContext {
>>   
>>       /**
>>        * Forced video codec_id.
> 
>> -     * Demuxing: Set by user.
>> +     * - demuxing: Set by user.
> 
> while at it, probably we should use "decoding" in place of demuxing,
> since in this file "decoding" is semantically equivalent and used most
> prominently, same below

My only immediate opinion about "decoding" is that it should go in a 
separate patch.  Functional vs. cosmetic changes and all that.

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