Hello, thanks for review, 1) Can you give some reason about why we shouldn't use errno? I think it is more clear to accept the full int32 range, even if min/max int32 values are very unlikely to be used. 2) The RFC 4566 don't give any limit on fmtp parameters values. In addition I can't find a spec that says fmtp integers parameters for AAC must be positive. So I don't think we should limit values to positive integers here as it would not be consistent to RFC.
On Sat, Jun 29, 2019 at 5:58 AM Reimar Döffinger <reimar.doeffin...@gmx.de> wrote: > I don't think we should be using errno when avoidable, and it is avoidable > here by disallowing min/max int32 values themselves. Or using strtoll. > I'm also rather sceptical about allowing negative values here, does that > make sense? > Admittedly the type is set to just "int", but maybe it should be unsigned > instead? > > On 28.06.2019, at 08:46, Olivier MAIGNIAL <olivier.maign...@smile.fr> > wrote: > > > Hello here! > > > > A simple ping about this patch > > If you have any question, feel free to ask! > > > > Regards, > > Olivier > > > > On Wed, Jun 19, 2019 at 3:38 PM Olivier Maignial < > olivier.maign...@smile.fr> > > wrote: > > > >> === PROBLEM === > >> > >> I was trying to record h264 + aac streams from an RTSP server to mp4 > file. > >> using this command line: > >> ffmpeg -v verbose -y -i "rtsp://<ip>/my_resources" -codec copy -bsf:a > >> aac_adtstoasc test.mp4 > >> > >> FFmpeg then fail to record audio and output this logs: > >> [rtsp @ 0xcda1f0] The profile-level-id field size is invalid (40) > >> [rtsp @ 0xcda1f0] Error parsing AU headers > >> ... > >> [rtsp @ 0xcda1f0] Could not find codec parameters for stream 1 > (Audio: > >> aac, 48000 Hz, 1 channels): unspecified sample format > >> > >> In SDP provided by my RTSP server I had this fmtp line: > >> a=fmtp:98 streamType=5; profile-level-id=40; mode=AAC-hbr; > >> config=1188; sizeLength=13; indexLength=3; indexDeltaLength=3; > >> > >> In FFmpeg code, I found a check introduced by commit > >> 24130234cd9dd733116d17b724ea4c8e12ce097a. It disallows values greater > than > >> 32 for fmtp line parameters. > >> However, In RFC-6416 (RTP Payload Format for MPEG-4 Audio/Visual > Streams) > >> give examples of "profile-level-id" values for AAC, up to 55. > >> Furthermore, RFC-4566 (SDP: Session Description Protocol) do not give > any > >> limit of size on interger parameters given in fmtp line. > >> > >> === FIX === > >> > >> Instead of prohibit values over 32, I propose to check the possible > >> integer overflow. > >> The use of strtol allow to check the string validity and the possible > >> overflow. > >> Value is then checked against INT32_MIN and INT32_MAX. Using > INT32_MIN/MAX > >> ensure to have the same behavior on 32 or 64 bits platforms. > >> > >> This patch fix my problem and I now can record my RTSP AAC stream to > mp4. > >> It has passed the full fate tests suite sucessfully. > >> > >> Signed-off-by: Olivier Maignial <olivier.maign...@smile.fr> > >> --- > >> > >> Changes V5 -> V6: > >> - Simplify code > >> > >> libavformat/rtpdec_mpeg4.c | 15 ++++++++++----- > >> 1 file changed, 10 insertions(+), 5 deletions(-) > >> > >> diff --git a/libavformat/rtpdec_mpeg4.c b/libavformat/rtpdec_mpeg4.c > >> index 4f70599..9c4f8a1 100644 > >> --- a/libavformat/rtpdec_mpeg4.c > >> +++ b/libavformat/rtpdec_mpeg4.c > >> @@ -289,15 +289,20 @@ static int parse_fmtp(AVFormatContext *s, > >> for (i = 0; attr_names[i].str; ++i) { > >> if (!av_strcasecmp(attr, attr_names[i].str)) { > >> if (attr_names[i].type == ATTR_NAME_TYPE_INT) { > >> - int val = atoi(value); > >> - if (val > 32) { > >> + char *end_ptr = NULL; > >> + errno = 0; > >> + long int val = strtol(value, &end_ptr, 10); > >> + if (end_ptr == value || end_ptr[0] != '\0' || > >> + errno == ERANGE || > >> + val < INT32_MIN || val > INT32_MAX) { > >> av_log(s, AV_LOG_ERROR, > >> - "The %s field size is invalid (%d)\n", > >> - attr, val); > >> + "The %s field value is not a valid > number, > >> or overflows int32: %s\n", > >> + attr, value); > >> return AVERROR_INVALIDDATA; > >> } > >> + > >> *(int *)((char *)data+ > >> - attr_names[i].offset) = val; > >> + attr_names[i].offset) = (int) val; > >> } else if (attr_names[i].type == ATTR_NAME_TYPE_STR) { > >> char *val = av_strdup(value); > >> if (!val) > >> -- > >> 2.7.4 > >> > >> > > _______________________________________________ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > To unsubscribe, visit link above, or email > > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".