[FFmpeg-devel] [PATCH v3 1/7] fftools/textformat: Extract and generalize textformat api from ffprobe.c

[Soft Works]

> -----Original Message-----
> From: Stefano Sabatini <stefasab at gmail.com>
> Sent: Samstag, 8. März 2025 15:37
> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> Cc: Soft Works <softworkz-at-hotmail.com at ffmpeg.org>; softworkz
> <softworkz at hotmail.com>; Andreas Rheinhardt
> <andreas.rheinhardt at outlook.com>
> Subject: Re: [FFmpeg-devel] [PATCH v3 1/7] fftools/textformat: Extract
> and generalize textformat api from ffprobe.c
> 
> On date Saturday 2025-03-01 10:01:58 +0000, softworkz wrote:
> [...]
> 
> > +int avtext_context_open(AVTextFormatContext **ptctx, const
> AVTextFormatter *formatter, AVTextWriterContext *writer, const char
> *args,
> > +                        const struct AVTextFormatSection *sections,
> int nb_sections,
> > +                        int show_value_unit,
> > +                        int use_value_prefix,
> > +                        int use_byte_value_binary_prefix,
> > +                        int use_value_sexagesimal_format,
> > +                        int show_optional_fields,
> > +                        char *show_data_hash);
> 
> writer -> writer_ctx?
> 
> I'm fine with changing this later to avoid massive rebase edits.

No problem, I realized that it's just a matter of tooling after getting annoyed by these things for years 😊
(done already)


> Also I notice there is some of the usual inconsistencies here:
> av_X_Y against avXY and avX_Y that we have in the rest of the code.
> 
> Maybe let's stick to avX_Y or to av_X_Y.
> 
> Also this might be:
> av_text_format_open(...)
> av_text_format_close(...)
> av_text_format_print_X(...)
> 
> Or to simplify we can just call the structure AVTextContext (I see
> text as an evolution of a string, meant for structured formatted data,
> which is implied by the fact that we need a formatter) and simplify
> related functions naming to:
> av_text_open(...)
> av_text_close(...)
> av_text_print_X(...)
> 
> av_text_formatter_...
> av_text_writer_open...
> av_text_writer_close...
> 
> In fact I don't think there is much gain in keeping "context" in the
> name of the functions.
> 
> What do you think?
> 
> Again, since this is not public API (yet?) this should not be
> considered a blocker (also I've been out of touch with FFmpeg and I
> might be not aware of API conventions evolution).

From bottom to top:

Regarding public API, I tried to name everything as if it would be public in order to make it easier and less invasive in case that would happen. I also set this as a goal (and originally submitted it, being in avutil) to make it fully independent.
Nicolas meant that it's not ready in the current form to become public and he's probably right about it. 
But in the new form and location it will be easy to work on it as the big refactoring change is out of the way now.


Naming: 

I think the word context is helpful to indicate what it is - like Codec and Codec-Context there's AVTextFormatter and AVTextFormatContext - imo it is good to understand the relation between the too.

For everything else I don't mind. I had changed the namings myself a number of times but each time it ended with another inconsistency - I was just kind of moving the inconsistency around.
So, I'll happily rename everything in whatever way is desired, I'd just say that before renaming we should make a full plan in advance which covers the full range.


Thanks
sw