[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 08:31
> 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 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.
> >
> 
> You completely misunderstand (or you try to be trolling me as your 😊
> indicates). "The issue" that can no longer happen with current master
> is
> the segfault after allocation failure. This could really happen before
> my fixes. The pointers to writer/formatter can still be NULL even on
> master (namely after allocation failure), therefore the checks need to
> be there.


I would never troll you in such a weird way. 
I just looked at a very wrong place, please take my apologies for 
the confusion.

Thanks,
sw