Re: [FFmpeg-devel] [PATCH 01/10] avformat/segment: Add segment_write_temp option

[Marton Balint <cus@passwd.hu>]

On Fri, 13 Jun 2025, softworkz . wrote:

> Hi Marton,
>
>
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>> Marton Balint
>> Sent: Freitag, 13. Juni 2025 22:38
>> To: FFmpeg development discussions and patches <ffmpeg-
>> devel@ffmpeg.org>
>> Subject: Re: [FFmpeg-devel] [PATCH 01/10] avformat/segment: Add
>> segment_write_temp option
>>
>>
>>
>> On Fri, 13 Jun 2025, softworkz wrote:
>>
>>> From: softworkz <softworkz@hotmail.com>
>>>
>>> Allows to write segments as temp files (.tmp) which
>>> are renamed on completion.
>>>
>>> Signed-off-by: softworkz <softworkz@hotmail.com>
>>> ---
>>> libavformat/segment.c | 30 +++++++++++++++++++++++++++---
>>> 1 file changed, 27 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/libavformat/segment.c b/libavformat/segment.c
>>> index 65323ec678..04e973a198 100644
>>> --- a/libavformat/segment.c
>>> +++ b/libavformat/segment.c
>>> @@ -121,6 +121,7 @@ typedef struct SegmentContext {
>>>     int   break_non_keyframes;
>>>     int   write_empty;
>>>
>>> +    int segment_write_temp; ///< write segments as temp files
>> and rename on completion
>>>     int use_rename;
>>>     char temp_list_filename[1024];
>>>
>>> @@ -226,6 +227,15 @@ static int
>> set_segment_filename(AVFormatContext *s)
>>>              seg->entry_prefix ? seg->entry_prefix : "",
>>>              av_basename(oc->url));
>>>
>>> +    // Write segment as a temp file and rename on completion
>>> +    if(seg->segment_write_temp) {
>>> +        av_strlcatf(buf, sizeof(buf), ".tmp");
>>> +        char *temp_name = av_strdup(buf);
>>> +        if (!temp_name)
>>> +            return AVERROR(ENOMEM);
>>
>> You should use av_asprintf() directly instead of strlcatf() +
>> av_strdup()
>
> I could be wrong, but I was thinking that this way is more efficient,
> because in the av_strdup case, the size of the needed allocation is
> already known, while I would suppose that av_asprintf() needs to
> allocate a larger size of memory because it cannot predict the eventual
> size after applying all the formats. Am I on a wrong track?

I think it does not matter, this is not performance critical code, so 
simplicity/readability is more imporant. Also we should avoid hard coded 
path length limits if we can, an by using a static buffer you can hit 
that.


>
>
>>> +        ff_format_set_url(oc, temp_name);
>>> +    }
>>> +
>>>     return 0;
>>> }
>>>
>>> @@ -372,7 +382,7 @@ static int segment_end(AVFormatContext *s,
>> int write_trailer, int is_last)
>>>             SegmentListEntry *entry = av_mallocz(sizeof(*entry));
>>>             if (!entry) {
>>>                 ret = AVERROR(ENOMEM);
>>> -                goto end;
>>> +                goto fail;
>>>             }
>>>
>>>             /* append new element */
>>> @@ -393,7 +403,7 @@ static int segment_end(AVFormatContext *s,
>> int write_trailer, int is_last)
>>>             }
>>>
>>>             if ((ret = segment_list_open(s)) < 0)
>>> -                goto end;
>>> +                goto fail;
>>>             for (entry = seg->segment_list_entries; entry; entry
>> = entry->next)
>>>                 segment_list_print_entry(seg->list_pb, seg-
>>> list_type, entry, s);
>>>             if (seg->list_type == LIST_TYPE_M3U8 && is_last)
>>> @@ -450,7 +460,20 @@ static int segment_end(AVFormatContext *s,
>> int write_trailer, int is_last)
>>>         }
>>>     }
>>>
>>> -end:
>>> +    ff_format_io_close(oc, &oc->pb);
>>> +
>>> +    // Now rename the .tmp file to its actual name.
>>> +    if (seg->segment_write_temp) {
>>> +        char *final_filename = av_strndup(oc->url, strlen(oc-
>>> url) - 4);
>>> +        if (!final_filename)
>>> +            return AVERROR(ENOMEM);
>>
>> goto fail?
>
> At that point,  ff_format_io_close(oc, &oc->pb) has
> already been called.

Ah, I missed that, OK then.

Thanks,
Marton

>
> Here's the full block (after the patch):
>
>
>    ff_format_io_close(oc, &oc->pb);
>
>    // Now rename the .tmp file to its actual name.
>    if (seg->segment_write_temp) {
>        char *final_filename = av_strndup(oc->url, strlen(oc->url) - 4);
>        if (!final_filename)
>            return AVERROR(ENOMEM);
>        ret = ff_rename(oc->url, final_filename, s);
>        av_free(final_filename);
>    }
>
>    return ret;
>
> fail:
>    ff_format_io_close(oc, &oc->pb);
>
>    return ret;
> }
>
>
> 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".
>
_______________________________________________
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".