[FFmpeg-devel] [PATCH] avformat/avio: Remove no-op code in url_find_protocol().

[Muhammad Faiz]

On Fri, Jul 7, 2017 at 11:27 AM, Wan-Teh Chang
<wtc-at-google.com at ffmpeg.org> wrote:
> Hi Muhammad,
>
> On Thu, Jul 6, 2017 at 7:56 PM, Muhammad Faiz <mfcc64 at gmail.com> wrote:
>> On Fri, Jul 7, 2017 at 9:18 AM, Wan-Teh Chang
>> <wtc-at-google.com at ffmpeg.org> wrote:
>>> In url_find_protocol(), proto_str is either "file" or a string
>>> consisting of only the characters in URL_SCHEME_CHARS, which does not
>>> include ','. Therefore the strchr(proto_str, ',') call always returns
>>> NULL.
>>>
>>> Note: The code was added in commit
>>> 6161c41817f6e53abb3021d67ca0f19def682718.
>>>
>>> Signed-off-by: Wan-Teh Chang <wtc at google.com>
>>> ---
>>>  libavformat/avio.c | 2 --
>>>  1 file changed, 2 deletions(-)
>>>
>>> diff --git a/libavformat/avio.c b/libavformat/avio.c
>>> index 1e79c9dd5c..64248e098b 100644
>>> --- a/libavformat/avio.c
>>> +++ b/libavformat/avio.c
>>> @@ -263,8 +263,6 @@ static const struct URLProtocol *url_find_protocol(const char *filename)
>>>          av_strlcpy(proto_str, filename,
>>>                     FFMIN(proto_len + 1, sizeof(proto_str)));
>>>
>>> -    if ((ptr = strchr(proto_str, ',')))
>>> -        *ptr = '\0';
>>
>> What about handling "subfile," ?
>
> I don't know what url_find_protocol() is intended to do, but I can
> show you what it actually does.
>
> Here is the relevant code in libavformat/avio.c:
>
> ======================================================================
> #define URL_SCHEME_CHARS                        \
>     "abcdefghijklmnopqrstuvwxyz"                \
>     "ABCDEFGHIJKLMNOPQRSTUVWXYZ"                \
>     "0123456789+-."
>
> static const struct URLProtocol *url_find_protocol(const char *filename)
> {
>     const URLProtocol **protocols;
>     char proto_str[128], proto_nested[128], *ptr;
>     size_t proto_len = strspn(filename, URL_SCHEME_CHARS);
>     int i;
>
>     if (filename[proto_len] != ':' &&
>         (strncmp(filename, "subfile,", 8) || !strchr(filename +
> proto_len + 1, ':')) ||
>         is_dos_path(filename))
>         strcpy(proto_str, "file");
>     else
>         av_strlcpy(proto_str, filename,
>                    FFMIN(proto_len + 1, sizeof(proto_str)));
>
>     if ((ptr = strchr(proto_str, ',')))
>         *ptr = '\0';
> ======================================================================
>
> Since I don't know how "subfile," should be handled by
> url_find_protocol(), I ran the following three test inputs in the
> debugger:
>
> If |filename| is "subfile,", then proto_len is 7 and the if branch
> copies "file" into proto_str.
>
> If |filename| is "subfile,abcdefg", then proto_len is 7 and the if
> branch copies "file" into proto_str.
>
> If |filename| is "subfile,abcdefg:hijk", then proto_len is 7 and the
> else branch copies "subfile" into proto_str.

Oh, I see. I was wrong.

>
> Is this the expected behavior?

I don't know. However it is the previous behavior, so LGTM.

Thank's.