| Message ID | c8283a23-83f5-bb8a-0a21-520ce0ce1bd3@gmail.com |
|---|---|
| State | New |
| Headers | show |
> On 10 Apr 2018, at 08:33, Jun Zhao <mypopydev@gmail.com> wrote: > > > <0001-lavf-aviobuf-add-ff_get_chomp_line.patch>_______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel patchset LGTM Thanks Steven
On Tue, 10 Apr 2018, Jun Zhao wrote: > > Maybe you should use ff_read_line_to_bprint instead? It already chops the trailing line endings, not any whitespace though. Generally I think we should deprecate ff_get_line, because line length limits always pop here or there... Regards, Marton
On 2018/4/10 21:54, Marton Balint wrote: > > > On Tue, 10 Apr 2018, Jun Zhao wrote: > >> >> > > Maybe you should use ff_read_line_to_bprint instead? It already chops > the trailing line endings, not any whitespace though. Generally I > think we should deprecate ff_get_line, because line length limits > always pop here or there... > ff_read_line_to_bprint usually use when we don't know the length limits and need to the caller free the AVBPrint resource, in hls case, I think this is a simple case, the other reason is I don't want to mix the AVBPrint and c native char * in hls. Thanks. > Regards, > Marton > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Wed, 11 Apr 2018, Jun Zhao wrote: > > > On 2018/4/10 21:54, Marton Balint wrote: >> >> >> On Tue, 10 Apr 2018, Jun Zhao wrote: >> >>> >>> >> >> Maybe you should use ff_read_line_to_bprint instead? It already chops >> the trailing line endings, not any whitespace though. Generally I >> think we should deprecate ff_get_line, because line length limits >> always pop here or there... >> > ff_read_line_to_bprint usually use when we don't know the length limits > and need to the caller free the AVBPrint resource, in hls case, I think > this is a simple case, the other reason is I don't want to mix the > AVBPrint and c native char * in hls. Thanks. Since it's just factorization of existing code, then I guess it is OK. But new code should not use either that or ff_get_line. (MAX_URL_SIZE being 4096 does not even adhere to the RFC recommendation of supporting at least 8000 char length URLs) https://tools.ietf.org/html/rfc7230#section-3.1.1 Regards, Marton
On 2018/4/11 16:52, Marton Balint wrote: > > > On Wed, 11 Apr 2018, Jun Zhao wrote: > >> >> >> On 2018/4/10 21:54, Marton Balint wrote: >>> >>> >>> On Tue, 10 Apr 2018, Jun Zhao wrote: >>> >>>> >>>> >>> >>> Maybe you should use ff_read_line_to_bprint instead? It already chops >>> the trailing line endings, not any whitespace though. Generally I >>> think we should deprecate ff_get_line, because line length limits >>> always pop here or there... >>> >> ff_read_line_to_bprint usually use when we don't know the length limits >> and need to the caller free the AVBPrint resource, in hls case, I think >> this is a simple case, the other reason is I don't want to mix the >> AVBPrint and c native char * in hls. Thanks. > > Since it's just factorization of existing code, then I guess it is OK. Yes > > But new code should not use either that or ff_get_line. (MAX_URL_SIZE > being 4096 does not even adhere to the RFC recommendation of > supporting at least 8000 char length URLs) I agree this part > > https://tools.ietf.org/html/rfc7230#section-3.1.1 I think HLS need to fix this issue when read URL. This is the other patchset I think. > > Regards, > Marton > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On 2018/4/11 16:52, Marton Balint wrote: > > > On Wed, 11 Apr 2018, Jun Zhao wrote: > >> >> >> On 2018/4/10 21:54, Marton Balint wrote: >>> >>> >>> On Tue, 10 Apr 2018, Jun Zhao wrote: >>> >>>> >>>> >>> >>> Maybe you should use ff_read_line_to_bprint instead? It already chops >>> the trailing line endings, not any whitespace though. Generally I >>> think we should deprecate ff_get_line, because line length limits >>> always pop here or there... >>> >> ff_read_line_to_bprint usually use when we don't know the length limits >> and need to the caller free the AVBPrint resource, in hls case, I think >> this is a simple case, the other reason is I don't want to mix the >> AVBPrint and c native char * in hls. Thanks. > > Since it's just factorization of existing code, then I guess it is OK. Pushed , thanks the comments, Marton. > > But new code should not use either that or ff_get_line. (MAX_URL_SIZE > being 4096 does not even adhere to the RFC recommendation of > supporting at least 8000 char length URLs) > > https://tools.ietf.org/html/rfc7230#section-3.1.1 > > Regards, > Marton > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c index 95b3364478..e752d0e1a6 100644 --- a/libavformat/aviobuf.c +++ b/libavformat/aviobuf.c @@ -823,6 +823,14 @@ int ff_get_line(AVIOContext *s, char *buf, int maxlen) return i; } +int ff_get_chomp_line(AVIOContext *s, char *buf, int maxlen) +{ + int len = ff_get_line(s, buf, maxlen); + while (len > 0 && av_isspace(buf[len - 1])) + buf[--len] = '\0'; + return len; +} + int64_t ff_read_line_to_bprint(AVIOContext *s, AVBPrint *bp) { int len, end; diff --git a/libavformat/internal.h b/libavformat/internal.h index c50382ad29..3582682925 100644 --- a/libavformat/internal.h +++ b/libavformat/internal.h @@ -299,6 +299,16 @@ void ff_put_v(AVIOContext *bc, uint64_t val); */ int ff_get_line(AVIOContext *s, char *buf, int maxlen); +/** + * Same as ff_get_line but strip the white-space characters in the text tail + * + * @param s the read-only AVIOContext + * @param buf buffer to store the read line + * @param maxlen size of the buffer + * @return the length of the string written in the buffer + */ +int ff_get_chomp_line(AVIOContext *s, char *buf, int maxlen); + /** * Read a whole line of text from AVIOContext to an AVBPrint buffer. Stop * reading after reaching a \\r, a \\n, a \\r\\n, a \\0 or EOF. The line