[FFmpeg-devel] [PATCH 01/12] fftools/textformat/avtextformat: Simplify avtext_print_rational()

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Andreas Rheinhardt
> Sent: Mittwoch, 16. April 2025 06:28
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 01/12]
> fftools/textformat/avtextformat: Simplify avtext_print_rational()
> 
> softworkz .:
> >
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> >> Andreas Rheinhardt
> >> Sent: Dienstag, 15. April 2025 10:36
> >> To: ffmpeg-devel at ffmpeg.org
> >> Subject: Re: [FFmpeg-devel] [PATCH 01/12]
> >> fftools/textformat/avtextformat: Simplify avtext_print_rational()
> >>
> >> softworkz .:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> >>>> Andreas Rheinhardt
> >>>> Sent: Dienstag, 15. April 2025 03:00
> >>>> To: FFmpeg development discussions and patches <ffmpeg-
> >>>> devel at ffmpeg.org>
> >>>> Subject: [FFmpeg-devel] [PATCH 01/12]
> >> fftools/textformat/avtextformat:
> >>>> Simplify avtext_print_rational()
> >>>>
> >>>> Patches attached.
> >>>>
> >>>> - Andreas
> >>>
> >>>
> >>> Hi Andreas,
> >>>
> >>> thanks a lot for working through this. I'll go over it tomorrow.
> >>>
> >>> As to not waste your time, it's probably best when we get those
> >>> changes applied in a timely manner so that I can rebase the new
> >>> patchset on top of it.
> >>>
> >>> Since you're sending the patches as attachments:
> >>> How do you want me to reply with code context? Whole files or
> >>> just snippets? And quoted?
> >>>
> >>
> >> Snippets is better. So is quoted.
> >>
> >> - Andreas
> >>
> >
> > Hi Andreas,
> >
> > thanks again for the well-spotted improvements. Just two notes:
> >
> >
> > 0007-fftools-textformat-Use-av_default_item_name.patch
> >
> > In the new patchset, those macros are removed from the individual
> > files. There's now a single macro in tf_internal.h and I've applied
> > this change there.
> 
> So you use a move/deduplicate commit to change something? Not good.

No. The patchset has a deduplication commit. That's what I've submitted
to the ML already. 
Now I made another commit locally (on top of that) which makes this change.


> > 0008-fftools-textformat-avtextformat-Fix-segfault-upon-al.patch
> > 0009-fftools-textformat-avtextformat-Fix-segfault-upon-al.patch
> >
> > Can this happen?
> 
> Of course it can. All allocations can fail. That's why we check them.
> Have you been coding with the assumption that allocations never fail?
> (You can use av_max_alloc(1); to simulate allocation failures.)

Allocations can fail, but statically initialized global const values?

> >
> > From my side, it would be great when we can get this merged asap,
> > then I'll post a rebased version of the patchset, also accounting
> > for your other review comment.
> 
> Done (with the exception of the av_default_item_name commit).
> 
> - Andreas

Awesome, thanks!

sw