Re: [FFmpeg-devel] [PATCH 3/3] avformat/hlsenc: Fix path handling for Windows

[Marton Balint <cus@passwd.hu>]

On Fri, 13 Jun 2025, softworkz . wrote:

>
>
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>> Marton Balint
>> Sent: Freitag, 13. Juni 2025 23:36
>> To: FFmpeg development discussions and patches <ffmpeg-
>> devel@ffmpeg.org>
>> Subject: Re: [FFmpeg-devel] [PATCH 3/3] avformat/hlsenc: Fix path
>> handling for Windows
>>
>>
>>
>> On Fri, 13 Jun 2025, softworkz wrote:
>>
>>> From: softworkz <softworkz@hotmail.com>
>>>
>>
>> Can you give an example where the path handling is wrong and where
>> this
>> patch fixes it?
>
> c:\hls\video1\master.m3u8

What I meant is that you should try to explain "fix" better in the 
commit message, like:

When base_output_dirname was determined only '/' was searched for as 
path separator. get_relative_url() on the other hand searched for both 
forward and backward slash regardless of OS.

Fix these issues by factorizing the separator finder function, only search 
for backslash for Windows/DOS and use that in both places.

>
>
>
>> Is there a trac ticket?
>
> Good question, there well may be.
>

Its worth doing a quick search, if there is one you might want to 
reference it in the commit message.

>
>
>>
>>> Signed-off-by: softworkz <softworkz@hotmail.com>
>>> ---
>>> libavformat/hlsenc.c | 43 ++++++++++++++++++++++++++++-----------
>> ----
>>> 1 file changed, 28 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
>>> index f81385d0b4..ba1e74e999 100644
>>> --- a/libavformat/hlsenc.c
>>> +++ b/libavformat/hlsenc.c
>>> @@ -329,6 +329,23 @@ static int hlsenc_io_close(AVFormatContext
>> *s, AVIOContext **pb, char *filename)
>>>     return ret;
>>> }
>>>
>>> +static int get_last_separator_pos(const char *path)
>>
>> size_t
>
> Cannot return -1.

Indeed, actually ptrdiff_t would be the proper type.

>
>
>>> +{
>>> +    if (!path || *path == '\0')
>>> +        return -1;
>>> +
>>> +    char *p = strrchr(path, '/');
>>> +#if HAVE_DOS_PATHS
>>> +    char *q = strrchr(path, '\\');
>>> +    p = FFMAX(p, q);
>>
>> You are comparing potentially NULL pointers here.
>
>
> It's the same like in av_basename() or av_dirname()

And those are likely wrong too.

>
>
>
>>> +#endif
>>> +
>>> +    if (!p)
>>> +        return -1;
>>> +
>>> +    return p - path;
>>> +}
>>> +
>>> static void set_http_options(AVFormatContext *s, AVDictionary
>> **options, HLSContext *c)
>>> {
>>>     int http_base_proto = ff_is_http_proto(s->url);
>>> @@ -1408,14 +1425,10 @@ static int
>> hls_rename_temp_file(AVFormatContext *s, AVFormatContext *oc)
>>>
>>> static const char* get_relative_url(const char *master_url, const
>> char *media_url)
>>> {
>>> -    const char *p = strrchr(master_url, '/');
>>> -    size_t base_len = 0;
>>> -
>>> -    if (!p) p = strrchr(master_url, '\\');
>>> +    int pos = get_last_separator_pos(master_url);
>>
>> size_t, and you can keep using base_len variable, you don't have to
>> rename, it is used for the same purpose as before.
>
>
>>
>>>
>>> -    if (p) {
>>> -        base_len = p - master_url;
>>> -        if (av_strncasecmp(master_url, media_url, base_len)) {
>>> +    if (pos >= 0) {
>
> That's the check for not being -1
>
> int64_t should cover the whole range - can I use that instead?

ptrdiff_t.

Thanks,
Marton
_______________________________________________
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".