[FFmpeg-devel] [PATCH 1/5] avformat/rtsp: make use of avio_get_str() to load the sdp

[Marton Balint]

On Wed, 1 Dec 2021, lance.lmwang at gmail.com wrote:

> On Wed, Dec 01, 2021 at 05:17:08PM +0200, Martin Storsjö wrote:
>> On Wed, 1 Dec 2021, lance.lmwang at gmail.com wrote:
>> 
>> > On Wed, Dec 01, 2021 at 03:55:37PM +0200, Martin Storsjö wrote:
>> > > On Wed, 1 Dec 2021, lance.lmwang at gmail.com wrote:
>> > > 
>> > > > From: Limin Wang <lance.lmwang at gmail.com>
>> > > > > Signed-off-by: Limin Wang <lance.lmwang at gmail.com>
>> > > > ---
>> > > > libavformat/rtsp.c | 30 ++++++++++++++++++++----------
>> > > > 1 file changed, 20 insertions(+), 10 deletions(-)
>> > > > > diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
>> > > > index e6a4993..ec8be8b 100644
>> > > > --- a/libavformat/rtsp.c
>> > > > +++ b/libavformat/rtsp.c
>> > > > @@ -2369,11 +2369,29 @@ static void append_source_addrs(char *buf, int size, const char *name,
>> > > >         av_strlcatf(buf, size, ",%s", addrs[i]->addr);
>> > > > }
>> > > > > +static char *read_sdp_str(AVIOContext *pb, int size)
>> > > > +{
>> > > > +    int n;
>> > > > +    char *str;
>> > > > +
>> > > > +    if (size < 0 || size == INT_MAX)
>> > > > +        return NULL;
>> > > > +
>> > > > +    str = av_malloc(size + 1);
>> > > > +    if (!str)
>> > > > +        return NULL;
>> > > > +    n = avio_get_str(pb, size, str, size + 1);
>> > > > +    if (n < size)
>> > > > +        avio_skip(pb, size - n);
>> > > 
>> > > What? Why do you want to skip past data in the input? Size here is just a
>> > > random static upper limit of how large an SDP file would be.
>> > yes, my fault, don't need to run avio_skip(). For avio_get_str() will return error only
>> > buflen <=0, so str is valid anyway.
>> > 
>> > > 
>> > > This patch totally lacks a motivation of why you want to do this.
>> > 
>> > considering the SDP is strings in file, so I think it's better to use avio_get_str().
>> 
>> Maybe, but warranting a patch changing things on its own, IMO.
>> 
>> > Also, I'm considering to remove limit of SDP_MAX_SIZE after that.
>> 
>> If you'd defer this patch to a patchset that does that, it might make more
>> sense. But I don't see much value in applying it on its own.
>
> Yes, I consider to add new api which is avio_get_str_len() to get the string len from pb, 
> before that, I want to hear no objection to use avio_get_str(). then I'll submit a patchset 
> for the complete change for review.

I thought you simply want to read the file to memory. You don't need new 
API function to do that, simply use avio_read_to_bprint() with INT_MAX 
upper limit. It will allocate the necessary buffer for you, and you can 
check if ENOMEM happened with av_bprint_is_complete(), and free the buffer 
after parsing with av_bprint_finalize().

Regards,
Marton