avformat/hlsenc: Some HLS improvements #97
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
Author
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
![ffmpeg-codebot[bot]](https://avatars.githubusercontent.com/in/158557?s=60&v=4){kind=small}
| ret = ffurl_shutdown(http_url_context, AVIO_FLAG_WRITE); | ||
| #endif | ||
| } | ||
| return ret; |
There was a problem hiding this comment.
On the FFmpeg mailing list, Marton Balint wrote (reply to this):
On Fri, 13 Jun 2025, softworkz wrote:
> From: softworkz <[email protected]>
>
Can you give an example where the path handling is wrong and where this
patch fixes it? Is there a trac ticket?
> Signed-off-by: softworkz <[email protected]>
> ---
> 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
> +{
> + 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.
> +#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) {
> + if (av_strncasecmp(master_url, media_url, pos)) {
> av_log(NULL, AV_LOG_WARNING, "Unable to find relative url\n");
> return NULL;
> }
> @@ -1423,7 +1436,7 @@ static const char* get_relative_url(const char *master_url, const char *media_ur
> return media_url;
> }
>
> - return media_url + base_len + 1;
> + return media_url + pos + 1;
> }
>
> static int64_t get_stream_bit_rate(AVStream *stream)
> @@ -3151,13 +3164,13 @@ static int hls_init(AVFormatContext *s)
> vs->fmp4_init_filename = expanded;
> }
>
> - p = strrchr(vs->m3u8_name, '/');
> - if (p) {
> - char tmp = *(++p);
> - *p = '\0';
> - vs->base_output_dirname = av_asprintf("%s%s", vs->m3u8_name,
> - vs->fmp4_init_filename);
> - *p = tmp;
> + int pos = get_last_separator_pos(vs->m3u8_name);
> + if (pos >= 0) {
> + AVBPrint buf;
> + av_bprint_init(&buf, 0, AV_BPRINT_SIZE_UNLIMITED);
> + av_bprint_append_data(&buf, vs->m3u8_name, pos + 1);
> + av_bprintf(&buf, "%s", vs->fmp4_init_filename);
> + av_bprint_finalize(&buf, &vs->base_output_dirname);
av_bprintf here is unneeded, because in the end you have to malloc the
result anyway. Keep using av_asprintf() with "%*s" to limit the length.
Regards,
Marton
> } else {
> vs->base_output_dirname = av_strdup(vs->fmp4_init_filename);
> }
> --
> ffmpeg-codebot
> _______________________________________________
> ffmpeg-devel mailing list
> [email protected]
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> [email protected] with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
[email protected]
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
[email protected] with subject "unsubscribe".
There was a problem hiding this comment.
On the FFmpeg mailing list, "softworkz ." wrote (reply to this):
> -----Original Message-----
> From: ffmpeg-devel <[email protected]> On Behalf Of
> Marton Balint
> Sent: Freitag, 13. Juni 2025 23:36
> To: FFmpeg development discussions and patches <ffmpeg-
> [email protected]>
> Subject: Re: [FFmpeg-devel] [PATCH 3/3] avformat/hlsenc: Fix path
> handling for Windows
>
>
>
> On Fri, 13 Jun 2025, softworkz wrote:
>
> > From: softworkz <[email protected]>
> >
>
> Can you give an example where the path handling is wrong and where
> this
> patch fixes it?
c:\hls\video1\master.m3u8
> Is there a trac ticket?
Good question, there well may be.
>
> > Signed-off-by: softworkz <[email protected]>
> > ---
> > 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.
> > +{
> > + 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()
> > +#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?
> > + if (av_strncasecmp(master_url, media_url, pos)) {
> > av_log(NULL, AV_LOG_WARNING, "Unable to find relative
> url\n");
> > return NULL;
> > }
> > @@ -1423,7 +1436,7 @@ static const char* get_relative_url(const
> char *master_url, const char *media_ur
> > return media_url;
> > }
> >
> > - return media_url + base_len + 1;
> > + return media_url + pos + 1;
> > }
> >
> > static int64_t get_stream_bit_rate(AVStream *stream)
> > @@ -3151,13 +3164,13 @@ static int hls_init(AVFormatContext *s)
> > vs->fmp4_init_filename = expanded;
> > }
> >
> > - p = strrchr(vs->m3u8_name, '/');
> > - if (p) {
> > - char tmp = *(++p);
> > - *p = '\0';
> > - vs->base_output_dirname =
> av_asprintf("%s%s", vs->m3u8_name,
> > - vs-
> >fmp4_init_filename);
> > - *p = tmp;
> > + int pos = get_last_separator_pos(vs->m3u8_name);
> > + if (pos >= 0) {
> > + AVBPrint buf;
> > + av_bprint_init(&buf, 0,
> AV_BPRINT_SIZE_UNLIMITED);
> > + av_bprint_append_data(&buf, vs->m3u8_name,
> pos + 1);
> > + av_bprintf(&buf, "%s", vs-
> >fmp4_init_filename);
> > + av_bprint_finalize(&buf, &vs-
> >base_output_dirname);
>
> av_bprintf here is unneeded, because in the end you have to malloc
> the
> result anyway. Keep using av_asprintf() with "%*s" to limit the
> length.
Ah, right, that's better, will do.
Thanks again,
sw
_______________________________________________
ffmpeg-devel mailing list
[email protected]
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
[email protected] with subject "unsubscribe".
There was a problem hiding this comment.
On the FFmpeg mailing list, "softworkz ." wrote (reply to this):
> -----Original Message-----
> From: ffmpeg-devel <[email protected]> On Behalf Of
> Marton Balint
> Sent: Freitag, 13. Juni 2025 23:36
> To: FFmpeg development discussions and patches <ffmpeg-
> [email protected]>
> Subject: Re: [FFmpeg-devel] [PATCH 3/3] avformat/hlsenc: Fix path
> handling for Windows
>
>
>
> On Fri, 13 Jun 2025, softworkz wrote:
>
> > From: softworkz <[email protected]>
> >
>
> Can you give an example where the path handling is wrong and where
> this
> patch fixes it? Is there a trac ticket?
>
> > Signed-off-by: softworkz <[email protected]>
> > ---
> > libavformat/hlsenc.c | 43 ++++++++++++++++++++++++++++-----------
> ----
> > 1 file changed, 28 insertions(+), 15 deletions(-)
> > +{
> > + 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.
Hi Marton,
actually, there's a prequel to this patch, a simple form of which I had
submitted in 2022 and ended up in a discussion with Andreas:
https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2022-January/291657.html
Obviously, this wasn't about performance-critical code in any way, but
as Andreas had insisted so much on it at that time, and there was also
topic about null-pointer comparison, I actually wanted to go a very
different route this time.
Looking at av_basename() (when HAVE_DOS_PATHS is enabled), it seemed
to be easy to beat the tripled search by doing a single (reverse)
search for all three chars.
So I created a function av_strrcspn(). The naming should be correct,
while there's no strrcspn function in reality.
I tried various optimizations but I wasn't able to beta the triple
strrchar().
Code and benchmark are here, in case anybody is interested:
https://github.com/FFmpeg/FFmpeg/compare/master...softworkz:FFmpeg:test_av_strrcspn
I suspect that there's some parallelism/SIMD in strrchar() which
makes it faster than expected.
Finally, I ditched that way and resorted to doing the same/similar
like in av_basename().
Now, either both a wrong or both are right...
Best regards,
sw
_______________________________________________
ffmpeg-devel mailing list
[email protected]
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
[email protected] with subject "unsubscribe".
There was a problem hiding this comment.
On the FFmpeg mailing list, Marton Balint wrote (reply to this):
On Fri, 13 Jun 2025, softworkz . wrote:
>
>
>> -----Original Message-----
>> From: ffmpeg-devel <[email protected]> On Behalf Of
>> Marton Balint
>> Sent: Freitag, 13. Juni 2025 23:36
>> To: FFmpeg development discussions and patches <ffmpeg-
>> [email protected]>
>> Subject: Re: [FFmpeg-devel] [PATCH 3/3] avformat/hlsenc: Fix path
>> handling for Windows
>>
>>
>>
>> On Fri, 13 Jun 2025, softworkz wrote:
>>
>>> From: softworkz <[email protected]>
>>>
>>
>> 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 <[email protected]>
>>> ---
>>> 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
[email protected]
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
[email protected] with subject "unsubscribe".
There was a problem hiding this comment.
On the FFmpeg mailing list, "softworkz ." wrote (reply to this):
> -----Original Message-----
> From: ffmpeg-devel <[email protected]> On Behalf Of Marton
> Balint
> Sent: Samstag, 14. Juni 2025 17:35
> To: FFmpeg development discussions and patches <[email protected]>
> Subject: Re: [FFmpeg-devel] [PATCH 3/3] avformat/hlsenc: Fix path
> handling for Windows
>
> On Fri, 13 Jun 2025, softworkz . wrote:
>
> >> -----Original Message-----
> >> From: ffmpeg-devel <[email protected]> On Behalf Of
> >> Marton Balint
> >> Sent: Freitag, 13. Juni 2025 23:36
> >> To: FFmpeg development discussions and patches <ffmpeg-
> >> [email protected]>
> >> Subject: Re: [FFmpeg-devel] [PATCH 3/3] avformat/hlsenc: Fix path
> >> handling for Windows
> >>
> >> On Fri, 13 Jun 2025, softworkz wrote:
> >>
> >>> From: softworkz <[email protected]>
> >>
> >> 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.
Hi Marton,
thanks for the nice message, applied in v3.
> >> Is there a trac ticket?
> >
> > Good question, there well may be.
(The reason for the dumb answer was trac being inaccessible on that
day, but I wasn't sure about it while writing)
> Its worth doing a quick search, if there is one you might want to
> reference it in the commit message.
Did a search today for hlsenc, hls, windows, path, slash, backslash in
various combinations but couldn't find anything.
> >>> Signed-off-by: softworkz <[email protected]>
> >>> ---
> >>> 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.
Ah sure, updated in v3.
> >>> +{
> >>> + 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.
Okay. I've added conditions to only compare non-null pointers.
Will post v3 in a moment.
Thank you,
sw
_______________________________________________
ffmpeg-devel mailing list
[email protected]
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
[email protected] with subject "unsubscribe".
softworkz
force-pushed
the
submit_hls_logging
branch
from
99dfda2 to
93e1a50
Compare
Collaborator
Author
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
softworkz
force-pushed
the
submit_hls_logging
branch
from
93e1a50 to
ea1a45c
Compare
..to allow playlist paths like: c:\hls\video1\master.m3u8 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. Reviewed-by: Marton Balint <[email protected]> Signed-off-by: softworkz <[email protected]>
softworkz
force-pushed
the
submit_hls_logging
branch
from
ea1a45c to
485ffe8
Compare
Collaborator
Author
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Includes only a single patch now:
Versions
V2
(as per review by Marton - thanks!)
V3
(all as per review by Marton Balint - thanks!)
.