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

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

> 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