[FFmpeg-devel] [PATCH] avformat/hls: Fixes overwriting existing #EXT-X-PROGRAM-DATE-TIME value in HLS playlist Bug ID: 8989

Vignesh Ravichandran vignesh.ravichandran02 at gmail.com
Tue Dec 1 13:03:58 EET 2020


Ping Steven, Andreas, Andriy and others,

I have pushed a patch with the sscanf check (
http://ffmpeg.org/pipermail/ffmpeg-devel/2020-December/273185.html) and
also verified that the build passes (
https://patchwork.ffmpeg.org/project/ffmpeg/patch/20201201101040.28338-1-vignesh.ravichandran02@gmail.com/).
Could you please advise if this is acceptable?

Thanks,
Vignesh

On Fri, Nov 27, 2020 at 12:11 PM Steven Liu <lq at chinaffmpeg.org> wrote:

>
>
> > 2020年11月27日 下午2:36,Vignesh Ravichandran <
> vignesh.ravichandran02 at gmail.com> 写道:
> >
> > av_isdigit seems to only check if the integer is from 0 to 9. So values
> > above 9 would fail the check, for example: 2020 would fail the check.
> Also,
> > we will not be able to use av_isdigit for checking the double value.
> >
> > On Fri, Nov 27, 2020 at 7:41 AM Steven Liu <lq at chinaffmpeg.org> wrote:
> >
> >>
> >>
> >>> 2020年11月26日 下午10:21,Vignesh Ravichandran <
> >> vignesh.ravichandran02 at gmail.com> 写道:
> >>>
> >>> This is is due to the following behavior in the current code:
> >>> 1. The initial_prog_date_time gets set to the current local time
> >>> 2. The existing playlist (.m3u8) file gets parsed and the segments
> >> present are added to the variant stream
> >>> 3. The new segment is created and added
> >>> 4. The existing segments and the new segment are written to the
> playlist
> >> file. The initial_prog_date_time from point 1 is used for calculating
> >> "#EXT-X-PROGRAM-DATE-TIME" for the segments, which results in incorrect
> >> "#EXT-X-PROGRAM-DATE-TIME" values for existing segments
> >>>
> >>> The following approach fixes this bug:
> >>> 1. Add a new variable "discont_program_date_time" of type double to
> >> HLSSegment struct
> >>> 2. Store the "EXT-X-PROGRAM-DATE-TIME" value from the existing segments
> >> in this variable
> >>> 3. When writing to playlist file if "discont_program_date_time" is set,
> >> then use that value for "EXT-X-PROGRAM-DATE-TIME" else use the value
> >> present in vs->initial_prog_date_time
> >>>
> >>> Signed-off-by: Vignesh Ravichandran <vignesh.ravichandran02 at gmail.com>
> >>> ---
> >>> libavformat/hlsenc.c | 83 ++++++++++++++++++++++++++++++++++++++++++--
> >>> 1 file changed, 80 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> >>> index cbfd8f7c0d..9346b2cfea 100644
> >>> --- a/libavformat/hlsenc.c
> >>> +++ b/libavformat/hlsenc.c
> >>> @@ -25,6 +25,10 @@
> >>> #include <stdint.h>
> >>> #if HAVE_UNISTD_H
> >>> #include <unistd.h>
> >>> +#include <string.h>
> >>> +#include <errno.h>
> >>> +#include <limits.h>
> >>> +#include <math.h>
> >>> #endif
> >>>
> >>> #if CONFIG_GCRYPT
> >>> @@ -88,6 +92,7 @@ typedef struct HLSSegment {
> >>>    char iv_string[KEYSIZE*2 + 1];
> >>>
> >>>    struct HLSSegment *next;
> >>> +    double discont_program_date_time;
> >>> } HLSSegment;
> >>>
> >>> typedef enum HLSFlags {
> >>> @@ -1124,6 +1129,7 @@ static int hls_append_segment(struct
> >> AVFormatContext *s, HLSContext *hls,
> >>>    en->keyframe_size     = vs->video_keyframe_size;
> >>>    en->next     = NULL;
> >>>    en->discont  = 0;
> >>> +    en->discont_program_date_time = 0;
> >>>
> >>>    if (vs->discontinuity) {
> >>>        en->discont = 1;
> >>> @@ -1148,7 +1154,8 @@ static int hls_append_segment(struct
> >> AVFormatContext *s, HLSContext *hls,
> >>>
> >>>    if (hls->max_nb_segments && vs->nb_entries >= hls->max_nb_segments)
> {
> >>>        en = vs->segments;
> >>> -        vs->initial_prog_date_time += en->duration;
> >>> +        if (!en->next->discont_program_date_time &&
> >> !en->discont_program_date_time)
> >>> +            vs->initial_prog_date_time += en->duration;
> >>>        vs->segments = en->next;
> >>>        if (en && hls->flags & HLS_DELETE_SEGMENTS &&
> >>> #if FF_API_HLS_WRAP
> >>> @@ -1173,6 +1180,44 @@ static int hls_append_segment(struct
> >> AVFormatContext *s, HLSContext *hls,
> >>>    return 0;
> >>> }
> >>>
> >>> +static int check_program_date_time(const char *prog_date_time) {
> >>> +    char s[strlen(prog_date_time)], *sptr0, *sptr1, *sptr2, *sptr3;
> >>> +    char *err = NULL;
> >>> +    char *arr[6] = {NULL};
> >>> +    int i = 0;
> >>> +    av_strlcpy(s, prog_date_time, strlen(prog_date_time));
> >>> +    char *p = strtok_r(strtok_r(s, "+", &sptr0), "T", &sptr1);
> >>> +    while (p) {
> >>> +        char *q = strtok_r(p, "-", &sptr2);
> >>> +        while (q) {
> >>> +            char *r = strtok_r(q, ":", &sptr3);
> >>> +            while (r) {
> >>> +                arr[i] = r;
> >>> +                i++;
> >>> +                r = strtok_r(NULL, ":", &sptr3);
> >>> +            }
> >>> +            q = strtok_r(NULL, "-", &sptr2);
> >>> +        }
> >>> +        p = strtok_r(NULL, "T", &sptr1);
> >>> +    }
> >>> +
> >>> +    for (i=0; i < 5; i++) {
> >>> +        errno=0;
> >>> +        long number = strtol(arr[i], &err, 10);
> >>> +        if((arr[i] == err) || (errno == ERANGE && number == LONG_MIN)
> >> || (errno == ERANGE && number == LONG_MAX)
> >>> +            || (errno == EINVAL) || (errno != 0 && number == 0) ||
> >> (errno == 0 && arr[i] && *err != 0))
> >> What about use av_isdigit to check them?
> >>> +            return AVERROR_INVALIDDATA;
> >>> +    }
> >>> +
> >>> +    errno=0;
> >>> +    double number = strtod(arr[i], &err);
> >>> +    if((arr[i] == err) || (errno == ERANGE && number == HUGE_VALF) ||
> >> (errno == ERANGE && number == HUGE_VALL)
> >>> +        || (errno == EINVAL) || (errno != 0 && number == 0) || (errno
> >> == 0 && arr[i] && *err != 0))
> >>> +        return AVERROR_INVALIDDATA;
> >>> +
> >>> +    return 0;
> >>> +}
> >>> +
> >>> static int parse_playlist(AVFormatContext *s, const char *url,
> >> VariantStream *vs)
> >>> {
> >>>    HLSContext *hls = s->priv_data;
> >>> @@ -1182,6 +1227,8 @@ static int parse_playlist(AVFormatContext *s,
> >> const char *url, VariantStream *vs
> >>>    char line[MAX_URL_SIZE];
> >>>    const char *ptr;
> >>>    const char *end;
> >>> +    int is_discont_detected = 0;
> >>> +    double discont_program_date_time = 0;
> >>>
> >>>    if ((ret = ffio_open_whitelist(&in, url, AVIO_FLAG_READ,
> >>>                                   &s->interrupt_callback, NULL,
> >>> @@ -1211,6 +1258,7 @@ static int parse_playlist(AVFormatContext *s,
> >> const char *url, VariantStream *vs
> >>>        } else if (av_strstart(line, "#EXT-X-DISCONTINUITY", &ptr)) {
> >>>            is_segment = 1;
> >>>            vs->discontinuity = 1;
> >>> +            is_discont_detected = 1;
> >>>        } else if (av_strstart(line, "#EXTINF:", &ptr)) {
> >>>            is_segment = 1;
> >>>            vs->duration = atof(ptr);
> >>> @@ -1236,7 +1284,31 @@ static int parse_playlist(AVFormatContext *s,
> >> const char *url, VariantStream *vs
> >>>                    av_strlcpy(vs->iv_string, ptr,
> >> sizeof(vs->iv_string));
> >>>                }
> >>>            }
> >>> +        } else if (av_strstart(line, "#EXT-X-PROGRAM-DATE-TIME:",
> >> &ptr)) {
> >>> +            struct tm program_date_time;
> >>> +            int y,M,d,h,m,s;
> >>> +            double ms;
> >>> +
> >>> +            if (check_program_date_time(ptr) != 0) {
> >>> +                av_log(hls, AV_LOG_VERBOSE,
> >>> +                       "Found invalid program date time %s when
> parsing
> >> playlist. "
> >>> +                       "Current and subsequent existing segments will
> >> be ignored", ptr);
> >>> +                ret = AVERROR_INVALIDDATA;
> >>> +                goto fail;
> >>> +            }
> >>>
> >>> +            sscanf(ptr, "%d-%d-%dT%d:%d:%d.%lf", &y, &M, &d, &h, &m,
> >> &s, &ms);
>
> Ping Andreas & Other guys,
>
>         I cannot sure only check return value of sscanf is ok, or validate
> the string “ptr" before sscanf,
>         sscanf can return the result,  so do we need validate the string
> “ptr”?
>
>
> >>> +            program_date_time.tm_year = y - 1900;
> >>> +            program_date_time.tm_mon = M - 1;
> >>> +            program_date_time.tm_mday = d;
> >>> +            program_date_time.tm_hour = h;
> >>> +            program_date_time.tm_min = m;
> >>> +            program_date_time.tm_sec = s;
> >>> +            program_date_time.tm_isdst = -1;
> >>> +
> >>> +            discont_program_date_time = mktime(&program_date_time);
> >>> +            discont_program_date_time += (double)(ms / 1000);
> >>> +            is_discont_detected = 0;
> >>>        } else if (av_strstart(line, "#", NULL)) {
> >>>            continue;
> >>>        } else if (line[0]) {
> >>> @@ -1250,8 +1322,9 @@ static int parse_playlist(AVFormatContext *s,
> >> const char *url, VariantStream *vs
> >>>                is_segment = 0;
> >>>                new_start_pos = avio_tell(vs->avf->pb);
> >>>                vs->size = new_start_pos - vs->start_pos;
> >>> -                vs->initial_prog_date_time -= vs->duration; // this is
> >> a previously existing segment
> >>>                ret = hls_append_segment(s, hls, vs, vs->duration,
> >> vs->start_pos, vs->size);
> >>> +                vs->last_segment->discont_program_date_time =
> >> discont_program_date_time;
> >>> +                discont_program_date_time += vs->duration;
> >>>                if (ret < 0)
> >>>                    goto fail;
> >>>                vs->start_pos = new_start_pos;
> >>> @@ -1572,7 +1645,11 @@ static int hls_window(AVFormatContext *s, int
> >> last, VariantStream *vs)
> >>>        ret = ff_hls_write_file_entry(byterange_mode ? hls->m3u8_out :
> >> vs->out, en->discont, byterange_mode,
> >>>                                      en->duration, hls->flags &
> >> HLS_ROUND_DURATIONS,
> >>>                                      en->size, en->pos, hls->baseurl,
> >>> -                                      en->filename, prog_date_time_p,
> >> en->keyframe_size, en->keyframe_pos, hls->flags & HLS_I_FRAMES_ONLY);
> >>> +                                      en->filename,
> >>> +                                      en->discont_program_date_time ?
> >> &en->discont_program_date_time : prog_date_time_p,
> >>> +                                      en->keyframe_size,
> >> en->keyframe_pos, hls->flags & HLS_I_FRAMES_ONLY);
> >>> +        if (en->discont_program_date_time)
> >>> +            en->discont_program_date_time -= en->duration;
> >>>        if (ret < 0) {
> >>>            av_log(s, AV_LOG_WARNING, "ff_hls_write_file_entry get
> >> error\n");
> >>>        }
> >>> --
> >>> 2.24.2 (Apple Git-127)
> >>>
> >>> _______________________________________________
> >>> ffmpeg-devel mailing list
> >>> ffmpeg-devel at ffmpeg.org
> >>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>>
> >>> To unsubscribe, visit link above, or email
> >>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> >>
> >> Thanks
> >>
> >> Steven Liu
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>
> Thanks
>
> Steven Liu
>
>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".


More information about the ffmpeg-devel mailing list