| Message ID | 20240229162424.850294-2-ffmpeg-devel@pileofstuff.org |
|---|---|
| State | New |
| Headers | show |
| Series | Harmonise comments about ownership/meaning | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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". >
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. */ enum AVCodecID video_codec_id; /** * Forced audio codec_id. - * Demuxing: Set by user. + * - demuxing: Set by user. */ enum AVCodecID audio_codec_id; /** * Forced subtitle codec_id. - * Demuxing: Set by user. + * - demuxing: Set by user. */ enum AVCodecID subtitle_codec_id; @@ -1622,11 +1623,11 @@ typedef struct AVFormatContext { /** * Custom interrupt callbacks for the I/O layer. * - * demuxing: set by the user before avformat_open_input(). - * muxing: set by the user before avformat_write_header() - * (mainly useful for AVFMT_NOFILE formats). The callback - * should also be passed to avio_open2() if it's used to - * open the file. + * - demuxing: set by the user before avformat_open_input(). + * - muxing: set by the user before avformat_write_header() + * (mainly useful for AVFMT_NOFILE formats). The callback + * should also be passed to avio_open2() if it's used to + * open the file. */ AVIOInterruptCB interrupt_callback; @@ -1828,7 +1829,7 @@ typedef struct AVFormatContext { * Forced video codec. * This allows forcing a specific decoder, even when there are multiple with * the same codec_id. - * Demuxing: Set by user + * - demuxing: Set by user */ const struct AVCodec *video_codec; @@ -1836,7 +1837,7 @@ typedef struct AVFormatContext { * Forced audio codec. * This allows forcing a specific decoder, even when there are multiple with * the same codec_id. - * Demuxing: Set by user + * - demuxing: Set by user */ const struct AVCodec *audio_codec; @@ -1844,7 +1845,7 @@ typedef struct AVFormatContext { * Forced subtitle codec. * This allows forcing a specific decoder, even when there are multiple with * the same codec_id. - * Demuxing: Set by user + * - demuxing: Set by user */ const struct AVCodec *subtitle_codec; @@ -1852,14 +1853,14 @@ typedef struct AVFormatContext { * Forced data codec. * This allows forcing a specific decoder, even when there are multiple with * the same codec_id. - * Demuxing: Set by user + * - demuxing: Set by user */ const struct AVCodec *data_codec; /** * Number of bytes to be written as padding in a metadata header. - * Demuxing: Unused. - * Muxing: Set by user. + * - demuxing: Unused. + * - muxing: Set by user. */ int metadata_header_padding; @@ -1876,7 +1877,7 @@ typedef struct AVFormatContext { /** * Output timestamp offset, in microseconds. - * Muxing: set by user + * - muxing: set by user */ int64_t output_ts_offset; @@ -1890,7 +1891,7 @@ typedef struct AVFormatContext { /** * Forced Data codec_id. - * Demuxing: Set by user. + * - demuxing: Set by user. */ enum AVCodecID data_codec_id;
There seems to be a convention for documenting ownership: /** * - 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(-)