[FFmpeg-devel] [Patch] Fix for ticket 6658 (Dash demuxer segfault)

Moritz Barsnick barsnick at gmx.net
Sun Nov 12 23:31:31 EET 2017


On Thu, Nov 09, 2017 at 00:19:33 +0000, Colin NG wrote:

Before the next attempt, please do have a look at the docs about
ffmpeg's programming style, especially bracket placement.

> +static int isLocal(char *url) {
> +
> +    if (av_strstart(url, "http://", NULL) || av_strstart(url, "https://", NULL))

Apart from the fact that I'm not sure whether such a function doesn't
already exist:

> +    {
> +        return FALSE;
> +    }
> +
> +    return TRUE;
> +}

TRUE/FALSE? Are you sure? Apart from that, a ternary check would be
simpler.

> -    ret = s->io_open(s, pb, url, AVIO_FLAG_READ, &tmp);
> +    {
> +        av_freep(pb);
> +        AVDictionary *opts = NULL;
> +        set_httpheader_options(c, opts);
> +        ret = avio_open2(pb, url, AVIO_FLAG_READ, c->interrupt_callback, &opts);
> +        av_dict_free(&opts);
> +        if (ret < 0)
> +            return ret;
> +    }

Why a separate block?

>      if (ret >= 0) {

And the return above obsoletes this check.

> +    char *path = malloc(MAX_URL_SIZE);

ffmpeg has its own malloc variants, and you MUST check the result.

> -    for (i = 0; i < n_baseurl_nodes; ++i) {
> +    for (i = 0; i < n_baseurl_nodes; i++) {

Why this change? (Yes, the latter is the more correct style, but
doesn't have anything to do with your patch.)

>      }
> +
>      if (rep_bandwidth_val && tmp_str[0] != '\0') {

Why this change?

> -    } else if (!av_strcasecmp(fragmenturl_node->name, (const char *)"SegmentURL")) {
> +    }
> +    else if (!av_strcasecmp(fragmenturl_node->name, (const char *)"SegmentURL")) {

Why this change?

>          }
> +
>          representation_segmenttemplate_node = find_child_node_by_name(representation_node, "SegmentTemplate");

Why this change?

> +            av_log(s, AV_LOG_INFO, "representation_segmentlist_node \n");

And why whitespace before the line break?

>                                                  adaptionset_baseurl_node);
> -            if (ret < 0) {
> +             if (ret < 0) {
>                  return ret;

Why this change?

>          close_in = 1;
> -
>          set_httpheader_options(c, opts);

Why this change?

> +        if ((mpd_baseurl_node = find_child_node_by_name(node, "BaseURL")) == NULL)
> +        {
> +            mpd_baseurl_node = xmlNewNode(node, "BaseURL");
> +         }

Watch your bracket style, and your indentation.


... and probably a lot of other issues which I cannot judge.

Moritz


More information about the ffmpeg-devel mailing list