diff mbox series

[FFmpeg-devel,6/6] avformat/hlsenc: Simplify setting base_output_dirname

Message ID 20200509191509.9812-6-andreas.rheinhardt@gmail.com
State Accepted
Commit 6db81e93a95d150ec828214ba7eb6183577c748c
Headers show
Series [FFmpeg-devel,1/6] avformat/hlsenc: Don't reset AVIOContext pointer manually a second time
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Andreas Rheinhardt May 9, 2020, 7:15 p.m. UTC
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
The usage of fmp4_init_filename_len is weird: It is basically used for
two different purposes: The length of vs->fmp4_init_filename_len and the
length of vs->base_output_dirname (this name is btw misleading because
it is not a dirname at all). And given that it's scope is the whole
function, the second time vs->fmp4_init_filename was allocated it also
contained enough space to contain the whole of vs->m3u8_name.

Furthermore, there seems to be a misconception in the way the
fmp4_init_filename_len is calculated in case of more than one
varstreams: It is incremented by strlen("_%d"), yet the string is not
built by inserting "_%d" into the other string; instead if no %v is
present in the string and if there is more than varstream, then it is
created by inserting "_%d", where %d is replace by the actual index of
the varstream. And this can take more than three bytes. But it is not
dangerous: fmp4_init_filename_len is incremented by strlen("_%d") on
every iteration of the loop.

I also pondered using av_append_path_component() in the case that p is
not NULL below; yet this might swallow a "/" and I don't know if it
would make a difference. (Is it actually allowed to use an absolute path
for hls->fmp4_init_filename? If one does so, said path will still be
treated as relative to the directory of vs->m3u8_name.)

I can also give vs->fmp4_init_filename a similar treatment; yet before
doing so I'd like to know if it is intentional that a "%v" in
hls->fmp4_init_filename only gets replaced if there is more than one
varstream (in other cases a %v also gets replaced when one only has one
varstream).

 libavformat/hlsenc.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

Comments

Steven Liu May 10, 2020, 2:24 a.m. UTC | #1
> 2020年5月10日 上午3:15,Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道:
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> The usage of fmp4_init_filename_len is weird: It is basically used for
> two different purposes: The length of vs->fmp4_init_filename_len and the
> length of vs->base_output_dirname (this name is btw misleading because
> it is not a dirname at all). And given that it's scope is the whole
> function, the second time vs->fmp4_init_filename was allocated it also
> contained enough space to contain the whole of vs->m3u8_name.
> 
> Furthermore, there seems to be a misconception in the way the
> fmp4_init_filename_len is calculated in case of more than one
> varstreams: It is incremented by strlen("_%d"), yet the string is not
> built by inserting "_%d" into the other string; instead if no %v is
> present in the string and if there is more than varstream, then it is
> created by inserting "_%d", where %d is replace by the actual index of
> the varstream. And this can take more than three bytes. But it is not
> dangerous: fmp4_init_filename_len is incremented by strlen("_%d") on
> every iteration of the loop.
> 
> I also pondered using av_append_path_component() in the case that p is
> not NULL below; yet this might swallow a "/" and I don't know if it
> would make a difference. (Is it actually allowed to use an absolute path
> for hls->fmp4_init_filename? If one does so, said path will still be
> treated as relative to the directory of vs->m3u8_name.)
> 
> I can also give vs->fmp4_init_filename a similar treatment; yet before
> doing so I'd like to know if it is intentional that a "%v" in
> hls->fmp4_init_filename only gets replaced if there is more than one
> varstream (in other cases a %v also gets replaced when one only has one
> varstream).
> 
> libavformat/hlsenc.c | 24 +++++++++---------------
> 1 file changed, 9 insertions(+), 15 deletions(-)
> 
> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> index d80852739e..be54957e9d 100644
> --- a/libavformat/hlsenc.c
> +++ b/libavformat/hlsenc.c
> @@ -2908,24 +2908,18 @@ static int hls_init(AVFormatContext *s)
>                         return ret;
>                 }
> 
> -                fmp4_init_filename_len = strlen(vs->m3u8_name) +
> -                    strlen(vs->fmp4_init_filename) + 1;
> -
> -                vs->base_output_dirname = av_malloc(fmp4_init_filename_len);
> -                if (!vs->base_output_dirname)
> -                    return AVERROR(ENOMEM);
> -
> -                av_strlcpy(vs->base_output_dirname, vs->m3u8_name,
> -                           fmp4_init_filename_len);
> -                p = strrchr(vs->base_output_dirname, '/');
> +                p = strrchr(vs->m3u8_name, '/');
>                 if (p) {
> -                    *(p + 1) = '\0';
> -                    av_strlcat(vs->base_output_dirname, vs->fmp4_init_filename,
> -                               fmp4_init_filename_len);
> +                    char tmp = *(++p);
> +                    *p = '\0';
> +                    vs->base_output_dirname = av_asprintf("%s%s", vs->m3u8_name,
> +                                                          vs->fmp4_init_filename);
> +                    *p = tmp;
>                 } else {
> -                    av_strlcpy(vs->base_output_dirname, vs->fmp4_init_filename,
> -                               fmp4_init_filename_len);
> +                    vs->base_output_dirname = av_strdup(vs->fmp4_init_filename);
>                 }
> +                if (!vs->base_output_dirname)
> +                    return AVERROR(ENOMEM);
>             }
>         }
> 
> -- 
> 2.20.1
> 
> _______________________________________________
> 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".

LGTM

Thanks

Steven Liu
diff mbox series

Patch

diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
index d80852739e..be54957e9d 100644
--- a/libavformat/hlsenc.c
+++ b/libavformat/hlsenc.c
@@ -2908,24 +2908,18 @@  static int hls_init(AVFormatContext *s)
                         return ret;
                 }
 
-                fmp4_init_filename_len = strlen(vs->m3u8_name) +
-                    strlen(vs->fmp4_init_filename) + 1;
-
-                vs->base_output_dirname = av_malloc(fmp4_init_filename_len);
-                if (!vs->base_output_dirname)
-                    return AVERROR(ENOMEM);
-
-                av_strlcpy(vs->base_output_dirname, vs->m3u8_name,
-                           fmp4_init_filename_len);
-                p = strrchr(vs->base_output_dirname, '/');
+                p = strrchr(vs->m3u8_name, '/');
                 if (p) {
-                    *(p + 1) = '\0';
-                    av_strlcat(vs->base_output_dirname, vs->fmp4_init_filename,
-                               fmp4_init_filename_len);
+                    char tmp = *(++p);
+                    *p = '\0';
+                    vs->base_output_dirname = av_asprintf("%s%s", vs->m3u8_name,
+                                                          vs->fmp4_init_filename);
+                    *p = tmp;
                 } else {
-                    av_strlcpy(vs->base_output_dirname, vs->fmp4_init_filename,
-                               fmp4_init_filename_len);
+                    vs->base_output_dirname = av_strdup(vs->fmp4_init_filename);
                 }
+                if (!vs->base_output_dirname)
+                    return AVERROR(ENOMEM);
             }
         }