[FFmpeg-devel] [PATCH] avformat/hls: remove redundant code

Richard Shaffer rshaffer at tunein.com
Wed Apr 18 04:01:54 EEST 2018


On Mon, Apr 16, 2018 at 10:24 PM, Steven Liu <lq at chinaffmpeg.org> wrote:

>
>
> > On 16 Apr 2018, at 08:37, Jun Zhao <mypopydev at gmail.com> wrote:
> >
> >
> >
> > On 2018/4/13 20:29, Steven Liu wrote:
> >> 2018-04-13 16:19 GMT+08:00 Jun Zhao <mypopydev at gmail.com>:
> >>>
> >>> On 2018/4/12 16:48, Steven Liu wrote:
> >>>> Signed-off-by: Steven Liu <lq at chinaffmpeg.org>
> >>>> ---
> >>>> libavformat/hls.c | 27 +++++++++------------------
> >>>> 1 file changed, 9 insertions(+), 18 deletions(-)
> >>>>
> >>>> diff --git a/libavformat/hls.c b/libavformat/hls.c
> >>>> index ae0545a086..74f0c2ccc5 100644
> >>>> --- a/libavformat/hls.c
> >>>> +++ b/libavformat/hls.c
> >>>> @@ -945,14 +945,8 @@ static struct segment *next_segment(struct
> playlist *pls)
> >>>>     return pls->segments[n];
> >>>> }
> >>>>
> >>>> -enum ReadFromURLMode {
> >>>> -    READ_NORMAL,
> >>>> -    READ_COMPLETE,
> >>>> -};
> >>>> -
> >>>> static int read_from_url(struct playlist *pls, struct segment *seg,
> >>>> -                         uint8_t *buf, int buf_size,
> >>>> -                         enum ReadFromURLMode mode)
> >>>> +                         uint8_t *buf, int buf_size)
> >>>> {
> >>>>     int ret;
> >>>>
> >>>> @@ -960,12 +954,9 @@ static int read_from_url(struct playlist *pls,
> struct segment *seg,
> >>>>     if (seg->size >= 0)
> >>>>         buf_size = FFMIN(buf_size, seg->size - pls->cur_seg_offset);
> >>>>
> >>>> -    if (mode == READ_COMPLETE) {
> >>>> -        ret = avio_read(pls->input, buf, buf_size);
> >>>> -        if (ret != buf_size)
> >>>> -            av_log(NULL, AV_LOG_ERROR, "Could not read complete
> segment.\n");
> >>>> -    } else
> >>>> -        ret = avio_read(pls->input, buf, buf_size);
> >>>> +    ret = avio_read(pls->input, buf, buf_size);
> >>>> +    if (ret != buf_size)
> >>>> +        av_log(NULL, AV_LOG_ERROR, "Could not read complete
> segment.\n");
> >>>>
> >>>>     if (ret > 0)
> >>>>         pls->cur_seg_offset += ret;
> >>>> @@ -1085,7 +1076,7 @@ static void intercept_id3(struct playlist *pls,
> uint8_t *buf,
> >>>>     while (1) {
> >>>>         /* see if we can retrieve enough data for ID3 header */
> >>>>         if (*len < ID3v2_HEADER_SIZE && buf_size >=
> ID3v2_HEADER_SIZE) {
> >>>> -            bytes = read_from_url(pls, seg, buf + *len,
> ID3v2_HEADER_SIZE - *len, READ_COMPLETE);
> >>>> +            bytes = read_from_url(pls, seg, buf + *len,
> ID3v2_HEADER_SIZE - *len);
> >>>>             if (bytes > 0) {
> >>>>
> >>>>                 if (bytes == ID3v2_HEADER_SIZE - *len)
> >>>> @@ -1137,7 +1128,7 @@ static void intercept_id3(struct playlist *pls,
> uint8_t *buf,
> >>>>
> >>>>             if (remaining > 0) {
> >>>>                 /* read the rest of the tag in */
> >>>> -                if (read_from_url(pls, seg, pls->id3_buf +
> id3_buf_pos, remaining, READ_COMPLETE) != remaining)
> >>>> +                if (read_from_url(pls, seg, pls->id3_buf +
> id3_buf_pos, remaining) != remaining)
> >>>>                     break;
> >>>>                 id3_buf_pos += remaining;
> >>>>                 av_log(pls->ctx, AV_LOG_DEBUG, "Stripped additional
> %d HLS ID3 bytes\n", remaining);
> >>>> @@ -1151,7 +1142,7 @@ static void intercept_id3(struct playlist *pls,
> uint8_t *buf,
> >>>>
> >>>>     /* re-fill buffer for the caller unless EOF */
> >>>>     if (*len >= 0 && (fill_buf || *len == 0)) {
> >>>> -        bytes = read_from_url(pls, seg, buf + *len, buf_size - *len,
> READ_NORMAL);
> >>>> +        bytes = read_from_url(pls, seg, buf + *len, buf_size - *len);
> >>>>
> >>>>         /* ignore error if we already had some data */
> >>>>         if (bytes >= 0)
> >>>> @@ -1311,7 +1302,7 @@ static int update_init_section(struct playlist
> *pls, struct segment *seg)
> >>>>     av_fast_malloc(&pls->init_sec_buf, &pls->init_sec_buf_size,
> sec_size);
> >>>>
> >>>>     ret = read_from_url(pls, seg->init_section, pls->init_sec_buf,
> >>>> -                        pls->init_sec_buf_size, READ_COMPLETE);
> >>>> +                        pls->init_sec_buf_size);
> >>> Didn't care ret < pls->init_sec_buf_size ?
> >> avio_read is full size read, so it will return error, or
> >> init_sec_buf_size, as your question, maybe it will happen then the
> >> read_from_url called avio_read_partiall.
> > Thanks the clarify, I think the patch is Ok now.
> pushed
> >>>>     ff_format_io_close(pls->parent, &pls->input);
> >>>>
> >>>>     if (ret < 0)
> >>>> @@ -1506,7 +1497,7 @@ reload:
> >>>>     }
> >>>>
> >>>>     seg = current_segment(v);
> >>>> -    ret = read_from_url(v, seg, buf, buf_size, READ_NORMAL);
> >>>> +    ret = read_from_url(v, seg, buf, buf_size);
> >>>>     if (ret > 0) {
> >>>>         if (just_opened && v->is_id3_timestamped != 0) {
> >>>>             /* Intercept ID3 tags here, elementary audio streams are
> required
> >>> LGTM except one question.
> >>> _______________________________________________
> >>> ffmpeg-devel mailing list
> >>> ffmpeg-devel at ffmpeg.org
> >>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >> _______________________________________________
> >> ffmpeg-devel mailing list
> >> ffmpeg-devel at ffmpeg.org
> >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> Thanks
> Steven
>
>
> I notice that after this change, the message "Could not read complete
segment" appears when reading the end of an HLS segment. This makes sense,
since the end of the segment will often not align with the buffer size.

The code seems to work fine, but the message is annoying. The log message
seems to be left over from when the code was changed to use the avio_ API
functions instead of ffurl_ functions. (
https://git.ffmpeg.org/gitweb/ffmpeg.git/commit/6fbfb20faf22d15c0c92d124e22eb8298fb10dcb)
I think maybe the check for ret != buf_size and the log message are no
longer useful. Should we remove them?

-Richard

>
>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>


More information about the ffmpeg-devel mailing list