[FFmpeg-devel,1/4] lavf/aviobuf: add ff_get_chomp_line

Submitted by Jun Zhao on April 10, 2018, 12:33 a.m.

Details

Message ID c8283a23-83f5-bb8a-0a21-520ce0ce1bd3@gmail.com
State New
Headers show

Commit Message

Jun Zhao April 10, 2018, 12:33 a.m.
From 58e8cb520eeeb727ee834ee81877db7c81fe089b Mon Sep 17 00:00:00 2001
From: Jun Zhao <mypopydev@gmail.com>
Date: Mon, 9 Apr 2018 23:05:42 +0800
Subject: [PATCH 1/4] lavf/aviobuf: add ff_get_chomp_line

Same as ff_get_line but strip the white-space characters in the
string tail.

Signed-off-by: Jun Zhao <mypopydev@gmail.com>
---
 libavformat/aviobuf.c  |  8 ++++++++
 libavformat/internal.h | 10 ++++++++++
 2 files changed, 18 insertions(+)

Comments

Steven Liu April 10, 2018, 2:46 a.m.
> 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
Marton Balint April 10, 2018, 1:54 p.m.
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
Jun Zhao April 11, 2018, 12:46 a.m.
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
Marton Balint April 11, 2018, 8:52 a.m.
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
Jun Zhao April 12, 2018, 12:18 a.m.
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
Jun Zhao April 12, 2018, 8:38 a.m.
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

Patch hide | download patch | download mbox

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