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

[Andreas Rheinhardt]

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.

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

> (yet I'm good with it)
> 
> 
> Everything else LGTM:
> 
> 0001-fftools-textformat-avtextformat-Simplify-avtext_prin.patch
> 0002-fftools-textformat-Use-not-for-lavu-headers.patch
> 0003-fftools-textformat-tw_avio-Remove-unused-private-cla.patch
> 0004-fftools-textformat-tw_avio-Don-t-flush-unnecessarily.patch
> 0005-fftools-textformat-tw_avio-Use-avio_closep.patch
> 0006-textformat-tw_avio-Remove-close_on_uninit-param-from.patch
> 
> 0010-fftools-textformat-avtextformat-Initialize-stuff-ear.patch
> 0011-fftools-textformat-avtextformat-Add-missing-AVERROR.patch
> 0012-fftools-textformat-avtextformat-Make-close-functions.patch
> 
> 
> 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