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

[Soft Works <softworkz-at-hotmail.com@ffmpeg.org>]

> -----Original Message-----
> From: Stefano Sabatini <stefasab@gmail.com>
> Sent: Mittwoch, 5. März 2025 21:21
> To: Soft Works <softworkz@hotmail.com>
> Cc: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>; Soft Works <softworkz-at-hotmail.com@ffmpeg.org>;
> Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> Subject: Re: [FFmpeg-devel] [PATCH v3 1/7] fftools/textformat: Extract
> and generalize textformat api from ffprobe.c
> 
> On date Sunday 2025-03-02 19:44:34 +0000, Soft Works wrote:
> >
> >
> > > -----Original Message-----
> > > From: Stefano Sabatini <stefasab@gmail.com>
> > > Sent: Sonntag, 2. März 2025 18:55
> > > To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> > > Cc: Soft Works <softworkz-at-hotmail.com@ffmpeg.org>; softworkz
> > > <softworkz@hotmail.com>; Andreas Rheinhardt
> > > <andreas.rheinhardt@outlook.com>
> > > Subject: Re: [FFmpeg-devel] [PATCH v3 1/7] fftools/textformat:
> Extract
> > > and generalize textformat api from ffprobe.c
> > >
> > > Hi,
> > >
> > > I'll have a look at this in the week. There are a few things which I
> > > want to doublecheck (there is some ad-hoc application logic which
> > > might be broken when generalizing this), but I like the overall
> > > direction of this changeset.
> >
> > Hi Stefano,
> >
> > thanks a lot for your interest in this changeset.
> >
> > I think I know what you are talking about with regards to possible
> breakage 😊.
> >
> > In this changeset, I wanted to keep the changes compact to reduce the
> number of lines which needs to be reviewed. But there's a preceding
> patchset
> >
> > "[FFmpeg-devel,v2,0/8,RFC] avtextformat: Transform text writing into
> an independent API"
> >
> > which allows to follow the changes step-by-step.
> >
> > The part you are probably talking about is done here:
> >
> >
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/01413dfbc0c60ee17c7937
> 0086abb88d78552929.1740718936.git.ffmpegagent@gmail.com/
> >
> >
> > I have removed the specific fields..
> >
> > unsigned int nb_section_packet; ///< number of the packet section
> > unsigned int nb_section_frame;  ///< number of the frame  section
> > unsigned int nb_section_packet_frame; ///< nb_section_packet or
> nb_section_frame
> >
> > ..and replaced it with a 2-dimensional array:
> >
> > unsigned int nb_item[SECTION_MAX_NB_LEVELS];
> > unsigned int
> nb_item_type[SECTION_MAX_NB_LEVELS][SECTION_MAX_NB_SECTIONS]; <= new
> >
> > While nb_item counts the total number of current items at a certain
> level, nb_item_type counts the number of items for each type separately.
> >
> > Then,
> >
> > WRITER_FLAG_PUT_PACKETS_AND_FRAMES_IN_SAME_CHAPTER
> >
> > is replaced with
> >
> > TEXTFORMAT_FLAG_SUPPORTS_MIXED_ARRAY_CONTENT
> >
> 
> > and instead of checking for the specific ID
> > (SECTION_ID_PACKETS_AND_FRAMES), this is done with a new flag
> > (SECTION_FLAG_NUMBERING_BY_TYPE). All that combined allows to
> > achieve the same result as before (separate numbering of mixed array
> > content sections).
> >
> >
> > Generally, it was done in a refactoring way (not a rewrite), so
> > nothing should have gotten lost, only the output writers are new
> > (aviowriter, stdoutwriter, bufferwriter).
> 
> Cool, I think this should work indeed.

Thanks for looking at it!



> > > Not sure if this is ffmpeg-ish but we might move the formats within
> a
> > > dedicated subdirectory to make more apparent the core/components
> > > distinction - anyway this is not a blocker.
> >
> > I'll do that, but it's not quite clear how you mean it, as they _are_
> in a subdirectory already (textformat).
> > Or do you mean two separate subdirectories (textformat and
> textwriters)?
> 
> What I mean is that we might move the text formats to a dedicated
> directory (e.g. fftools/textformat/formats/compact.c) but it's not
> blocking at all.

Originally, I had thought the similar in a way that it might not be "ffmpeg-ish"; actually I've even be afraid that somebody might object the textformat subfolder. 
The above (fftools/textformat/formats/) is how I'd personally organize it as well, but of course everybody needs to adapt in a community project.

Some more opinions would be helpful..


> 
> > Another question: shall I add your name/copyright line in the header
> > of the writer files? It's still your code (I assume), I've just
> > transformed it a bit.
> 
> Feel free to skip it, git blame does a better job at tracking the
> authorship and I don't mind a lot about copyright anyway.

Neither do I. (Well, there's not much rights to get anyway with a stupid pseudonym 😃).

Thanks,
sw



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