| Message ID | 1638420098-14559-1-git-send-email-lance.lmwang@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v2] avformat/rtsp: load the sdp file with avio_read_to_bprint() | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | fail | Make fate failed |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | fail | Make fate failed |
On Thu, 2 Dec 2021, lance.lmwang@gmail.com wrote: > From: Limin Wang <lance.lmwang@gmail.com> > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com> > --- > libavformat/rtsp.c | 23 +++++++++-------------- > 1 file changed, 9 insertions(+), 14 deletions(-) > > diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c > index e6a4993..249a019 100644 > --- a/libavformat/rtsp.c > +++ b/libavformat/rtsp.c > @@ -2373,9 +2373,10 @@ static int sdp_read_header(AVFormatContext *s) > { > RTSPState *rt = s->priv_data; > RTSPStream *rtsp_st; > - int size, i, err; > + int i, err; > char *content; > char url[MAX_URL_SIZE]; > + AVBPrint bp; > > if (!ff_network_init()) > return AVERROR(EIO); > @@ -2386,22 +2387,16 @@ static int sdp_read_header(AVFormatContext *s) > rt->lower_transport = RTSP_LOWER_TRANSPORT_CUSTOM; > > /* read the whole sdp file */ > - /* XXX: better loading */ > - content = av_malloc(SDP_MAX_SIZE); > - if (!content) { > + av_bprint_init(&bp, 0, AV_BPRINT_SIZE_UNLIMITED); > + err = avio_read_to_bprint(s->pb, &bp, INT_MAX); > + if (err < 0 ) { > ff_network_close(); > - return AVERROR(ENOMEM); > + av_bprint_finalize(&bp, NULL); > + return err; > } > - size = avio_read(s->pb, content, SDP_MAX_SIZE - 1); > - if (size <= 0) { > - av_free(content); > - ff_network_close(); > - return AVERROR_INVALIDDATA; > - } > - content[size] ='\0'; > - > + content = bp.str; > err = ff_sdp_parse(s, content); The content variable could be eliminated here too (to avoid leaving a dangling pointer local variable after freeing it). > - av_freep(&content); > + av_bprint_finalize(&bp, NULL); > if (err) goto fail; > > /* open each RTP stream */ > -- > 1.8.3.1 This patch seems ok to me - maybe it'd be good to mention in the commit message that this allows getting rid of the hardcoded max size here, to explain the "why" for the patch, not only the "what"? Would you be interested in getting rid of the same hardcoded max size in rtspdec.c too? // Martin
On Thu, Dec 02, 2021 at 10:04:37AM +0200, Martin Storsjö wrote: > On Thu, 2 Dec 2021, lance.lmwang@gmail.com wrote: > > > From: Limin Wang <lance.lmwang@gmail.com> > > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com> > > --- > > libavformat/rtsp.c | 23 +++++++++-------------- > > 1 file changed, 9 insertions(+), 14 deletions(-) > > > > diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c > > index e6a4993..249a019 100644 > > --- a/libavformat/rtsp.c > > +++ b/libavformat/rtsp.c > > @@ -2373,9 +2373,10 @@ static int sdp_read_header(AVFormatContext *s) > > { > > RTSPState *rt = s->priv_data; > > RTSPStream *rtsp_st; > > - int size, i, err; > > + int i, err; > > char *content; > > char url[MAX_URL_SIZE]; > > + AVBPrint bp; > > > > if (!ff_network_init()) > > return AVERROR(EIO); > > @@ -2386,22 +2387,16 @@ static int sdp_read_header(AVFormatContext *s) > > rt->lower_transport = RTSP_LOWER_TRANSPORT_CUSTOM; > > > > /* read the whole sdp file */ > > - /* XXX: better loading */ > > - content = av_malloc(SDP_MAX_SIZE); > > - if (!content) { > > + av_bprint_init(&bp, 0, AV_BPRINT_SIZE_UNLIMITED); > > + err = avio_read_to_bprint(s->pb, &bp, INT_MAX); > > + if (err < 0 ) { > > ff_network_close(); > > - return AVERROR(ENOMEM); > > + av_bprint_finalize(&bp, NULL); > > + return err; > > } > > - size = avio_read(s->pb, content, SDP_MAX_SIZE - 1); > > - if (size <= 0) { > > - av_free(content); > > - ff_network_close(); > > - return AVERROR_INVALIDDATA; > > - } > > - content[size] ='\0'; > > - > > + content = bp.str; > > err = ff_sdp_parse(s, content); > > The content variable could be eliminated here too (to avoid leaving a > dangling pointer local variable after freeing it). OK, will remove content. > > > - av_freep(&content); > > + av_bprint_finalize(&bp, NULL); > > if (err) goto fail; > > > > /* open each RTP stream */ > > -- > > 1.8.3.1 > > This patch seems ok to me - maybe it'd be good to mention in the commit > message that this allows getting rid of the hardcoded max size here, to > explain the "why" for the patch, not only the "what"? OK, will add below comment message: this allows getting rid of the hardcoded max size of SDP. > > Would you be interested in getting rid of the same hardcoded max size in > rtspdec.c too? OK, will submit a patch for that. > > // Martin >
diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c index e6a4993..249a019 100644 --- a/libavformat/rtsp.c +++ b/libavformat/rtsp.c @@ -2373,9 +2373,10 @@ static int sdp_read_header(AVFormatContext *s) { RTSPState *rt = s->priv_data; RTSPStream *rtsp_st; - int size, i, err; + int i, err; char *content; char url[MAX_URL_SIZE]; + AVBPrint bp; if (!ff_network_init()) return AVERROR(EIO); @@ -2386,22 +2387,16 @@ static int sdp_read_header(AVFormatContext *s) rt->lower_transport = RTSP_LOWER_TRANSPORT_CUSTOM; /* read the whole sdp file */ - /* XXX: better loading */ - content = av_malloc(SDP_MAX_SIZE); - if (!content) { + av_bprint_init(&bp, 0, AV_BPRINT_SIZE_UNLIMITED); + err = avio_read_to_bprint(s->pb, &bp, INT_MAX); + if (err < 0 ) { ff_network_close(); - return AVERROR(ENOMEM); + av_bprint_finalize(&bp, NULL); + return err; } - size = avio_read(s->pb, content, SDP_MAX_SIZE - 1); - if (size <= 0) { - av_free(content); - ff_network_close(); - return AVERROR_INVALIDDATA; - } - content[size] ='\0'; - + content = bp.str; err = ff_sdp_parse(s, content); - av_freep(&content); + av_bprint_finalize(&bp, NULL); if (err) goto fail; /* open each RTP stream */