[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 07:37
> 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: 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?
> 
> The pointers to said static objects are only set after having
> allocated
> the private context. So the issue can happen (not with current master)

Current master is on top of which I had applied your patches and 
there's no possible way for those pointers to be null.

That you are submitting a patch that is fixing a flaw in an
unmerged future patch is pretty cool, but was also beyond my range
of consideration. 😊

Thanks for the clarification.

sw