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

From: Soft Works <softworkz-at-hotmail.com@ffmpeg.org>
To: Stefano Sabatini <stefasab@gmail.com>,
	FFmpeg development discussions and patches
	<ffmpeg-devel@ffmpeg.org>
Cc: 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
Date: Sun, 2 Mar 2025 19:44:34 +0000
Message-ID: <DM8P223MB0365E170AB8689C2FFA184FABACE2@DM8P223MB0365.NAMP223.PROD.OUTLOOK.COM> (raw)
In-Reply-To: <Z8Sbc6cDaKH0ydq4@mariano>

> -----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/01413dfbc0c60ee17c79370086abb88d78552929.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).

> 
> On date Saturday 2025-03-01 10:01:58 +0000, softworkz wrote:
> > From: softworkz <softworkz@hotmail.com>
> >
> > Signed-off-by: softworkz <softworkz@hotmail.com>
> > ---
> >  fftools/textformat/avtextformat.c  | 671
> +++++++++++++++++++++++++++++
> >  fftools/textformat/avtextformat.h  | 171 ++++++++
> >  fftools/textformat/avtextwriters.h |  68 +++
> 
> >  fftools/textformat/tf_compact.c    | 282 ++++++++++++
> >  fftools/textformat/tf_default.c    | 145 +++++++
> >  fftools/textformat/tf_flat.c       | 174 ++++++++
> >  fftools/textformat/tf_ini.c        | 160 +++++++
> >  fftools/textformat/tf_json.c       | 215 +++++++++
> >  fftools/textformat/tf_xml.c        | 221 ++++++++++
> >  fftools/textformat/tw_avio.c       | 129 ++++++
> >  fftools/textformat/tw_buffer.c     |  92 ++++
> >  fftools/textformat/tw_stdout.c     |  82 ++++
> 
> 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)?

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.

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