[FFmpeg-devel] [PATCH] avformat/mux: Check pkt->stream_index before using it
This commit fixes two recent regressions both of which are about using
pkt->stream_index as index in an AVFormatContext's streams array before
actually comparing the value with the count of streams in said array.
96e5e6abb9851d7a26ba21703955d5826ac857c0 did this in
prepare_input_packet() and 64063512227c4c87a7d16a1076481dc6baf19841 did
likewise in write_packets_common().
Signed-off-by: Andreas Rheinhardt
---
The same error in the same file applied on the same day by two different
people. How unlikely.
libavformat/mux.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/libavformat/mux.c b/libavformat/mux.c
index 41659b19c9..f2de73af9b 100644
--- a/libavformat/mux.c
+++ b/libavformat/mux.c
@@ -761,12 +761,13 @@ static int check_packet(AVFormatContext *s, AVPacket *pkt)
static int prepare_input_packet(AVFormatContext *s, AVPacket *pkt)
{
+AVStream *st;
int ret;
-AVStream *st = s->streams[pkt->stream_index];
ret = check_packet(s, pkt);
if (ret < 0)
return ret;
+st = s->streams[pkt->stream_index];
#if !FF_API_COMPUTE_PKT_FIELDS2 || !FF_API_LAVF_AVCTX
/* sanitize the timestamps */
@@ -1176,10 +1177,11 @@ static int write_packets_from_bsfs(AVFormatContext *s,
AVStream *st, AVPacket *p
static int write_packets_common(AVFormatContext *s, AVPacket *pkt, int
interleaved)
{
-AVStream *st = s->streams[pkt->stream_index];
+AVStream *st;
int ret = prepare_input_packet(s, pkt);
if (ret < 0)
return ret;
+st = s->streams[pkt->stream_index];
ret = check_bitstream(s, st, pkt);
if (ret < 0)
--
2.20.1
___
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".
Re: [FFmpeg-devel] [PATCH] avformat/mux: Check pkt->stream_index before using it
Quoting Andreas Rheinhardt (2020-05-08 00:55:00) > This commit fixes two recent regressions both of which are about using > pkt->stream_index as index in an AVFormatContext's streams array before > actually comparing the value with the count of streams in said array. > 96e5e6abb9851d7a26ba21703955d5826ac857c0 did this in > prepare_input_packet() and 64063512227c4c87a7d16a1076481dc6baf19841 did > likewise in write_packets_common(). > > Signed-off-by: Andreas Rheinhardt > --- > The same error in the same file applied on the same day by two different > people. How unlikely. How is it a regression? Isn't it rather invalid API use? Not that I object to having a check. But then why is check_packet() called so deep and not immediately on entry to the muxer? -- Anton Khirnov ___ 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".
Re: [FFmpeg-devel] [PATCH] avformat/mux: Check pkt->stream_index before using it
On Sun, 10 May 2020, Anton Khirnov wrote: Quoting Andreas Rheinhardt (2020-05-08 00:55:00) This commit fixes two recent regressions both of which are about using pkt->stream_index as index in an AVFormatContext's streams array before actually comparing the value with the count of streams in said array. 96e5e6abb9851d7a26ba21703955d5826ac857c0 did this in prepare_input_packet() and 64063512227c4c87a7d16a1076481dc6baf19841 did likewise in write_packets_common(). Signed-off-by: Andreas Rheinhardt --- The same error in the same file applied on the same day by two different people. How unlikely. How is it a regression? Isn't it rather invalid API use? Fun fact: 7b03b65bf0d02519c86750d2da33f413e11cf0c6 Yes, it is kind of invalid API use, but since the check is already there, we should make it actually worthwile. Not that I object to having a check. But then why is check_packet() called so deep and not immediately on entry to the muxer? I guess it is not that deep, but recent factorization efforts hidden it a bit. Regards, Marton ___ 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".
Re: [FFmpeg-devel] [PATCH] avformat/mux: Check pkt->stream_index before using it
Quoting Marton Balint (2020-05-10 19:45:04) > > > On Sun, 10 May 2020, Anton Khirnov wrote: > > > Quoting Andreas Rheinhardt (2020-05-08 00:55:00) > >> This commit fixes two recent regressions both of which are about using > >> pkt->stream_index as index in an AVFormatContext's streams array before > >> actually comparing the value with the count of streams in said array. > >> 96e5e6abb9851d7a26ba21703955d5826ac857c0 did this in > >> prepare_input_packet() and 64063512227c4c87a7d16a1076481dc6baf19841 did > >> likewise in write_packets_common(). > >> > >> Signed-off-by: Andreas Rheinhardt > >> --- > >> The same error in the same file applied on the same day by two different > >> people. How unlikely. > > > > How is it a regression? Isn't it rather invalid API use? > > Fun fact: 7b03b65bf0d02519c86750d2da33f413e11cf0c6 > > Yes, it is kind of invalid API use, but since the check is already there, > we should make it actually worthwile. lol I agree that checking for it is a good idea, obviously, but I wouldn't call it a regression. > > > > > Not that I object to having a check. But then why is check_packet() > > called so deep and not immediately on entry to the muxer? > > I guess it is not that deep, but recent factorization efforts hidden it a > bit. You can see in my original commit it is the very first thing done after entering the muxer. Right now it's several function calls deep. -- Anton Khirnov ___ 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".
Re: [FFmpeg-devel] [PATCH] avformat/mux: Check pkt->stream_index before using it
Anton Khirnov: > Quoting Marton Balint (2020-05-10 19:45:04) >> >> >> On Sun, 10 May 2020, Anton Khirnov wrote: >> >>> Quoting Andreas Rheinhardt (2020-05-08 00:55:00) This commit fixes two recent regressions both of which are about using pkt->stream_index as index in an AVFormatContext's streams array before actually comparing the value with the count of streams in said array. 96e5e6abb9851d7a26ba21703955d5826ac857c0 did this in prepare_input_packet() and 64063512227c4c87a7d16a1076481dc6baf19841 did likewise in write_packets_common(). Signed-off-by: Andreas Rheinhardt --- The same error in the same file applied on the same day by two different people. How unlikely. >>> >>> How is it a regression? Isn't it rather invalid API use? >> >> Fun fact: 7b03b65bf0d02519c86750d2da33f413e11cf0c6 >> >> Yes, it is kind of invalid API use, but since the check is already there, >> we should make it actually worthwile. > > lol > > I agree that checking for it is a good idea, obviously, but I wouldn't > call it a regression. > How about rephrasing the first sentence to: "This commit stops using pkt->stream_index as index in an AVFormatContext's streams array before actually comparing the value with the count of streams in said array." >> >>> >>> Not that I object to having a check. But then why is check_packet() >>> called so deep and not immediately on entry to the muxer? >> >> I guess it is not that deep, but recent factorization efforts hidden it a >> bit. > > You can see in my original commit it is the very first thing done after > entering the muxer. Right now it's several function calls deep. > I could make it the very first thing called in write_packets_common(). - Andreas ___ 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".
Re: [FFmpeg-devel] [PATCH] avformat/mux: Check pkt->stream_index before using it
Quoting Andreas Rheinhardt (2020-05-10 21:35:54) > Anton Khirnov: > > Quoting Marton Balint (2020-05-10 19:45:04) > >> > >> > >> On Sun, 10 May 2020, Anton Khirnov wrote: > >> > >>> Quoting Andreas Rheinhardt (2020-05-08 00:55:00) > This commit fixes two recent regressions both of which are about using > pkt->stream_index as index in an AVFormatContext's streams array before > actually comparing the value with the count of streams in said array. > 96e5e6abb9851d7a26ba21703955d5826ac857c0 did this in > prepare_input_packet() and 64063512227c4c87a7d16a1076481dc6baf19841 did > likewise in write_packets_common(). > > Signed-off-by: Andreas Rheinhardt > --- > The same error in the same file applied on the same day by two different > people. How unlikely. > >>> > >>> How is it a regression? Isn't it rather invalid API use? > >> > >> Fun fact: 7b03b65bf0d02519c86750d2da33f413e11cf0c6 > >> > >> Yes, it is kind of invalid API use, but since the check is already there, > >> we should make it actually worthwile. > > > > lol > > > > I agree that checking for it is a good idea, obviously, but I wouldn't > > call it a regression. > > > How about rephrasing the first sentence to: "This commit stops using > pkt->stream_index as index in an AVFormatContext's streams array before > actually comparing the value with the count of streams in said array." Sure, sounds good. > >> > >>> > >>> Not that I object to having a check. But then why is check_packet() > >>> called so deep and not immediately on entry to the muxer? > >> > >> I guess it is not that deep, but recent factorization efforts hidden it a > >> bit. > > > > You can see in my original commit it is the very first thing done after > > entering the muxer. Right now it's several function calls deep. > > > I could make it the very first thing called in write_packets_common(). Why not move the check_packet() call out of prepare_packet() into av_[interleaved_]write_frame() instead? -- Anton Khirnov ___ 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".
Re: [FFmpeg-devel] [PATCH] avformat/mux: Check pkt->stream_index before using it
Anton Khirnov: > Quoting Andreas Rheinhardt (2020-05-10 21:35:54) >> Anton Khirnov: >>> Quoting Marton Balint (2020-05-10 19:45:04) On Sun, 10 May 2020, Anton Khirnov wrote: > Quoting Andreas Rheinhardt (2020-05-08 00:55:00) >> This commit fixes two recent regressions both of which are about using >> pkt->stream_index as index in an AVFormatContext's streams array before >> actually comparing the value with the count of streams in said array. >> 96e5e6abb9851d7a26ba21703955d5826ac857c0 did this in >> prepare_input_packet() and 64063512227c4c87a7d16a1076481dc6baf19841 did >> likewise in write_packets_common(). >> >> Signed-off-by: Andreas Rheinhardt >> --- >> The same error in the same file applied on the same day by two different >> people. How unlikely. > > How is it a regression? Isn't it rather invalid API use? Fun fact: 7b03b65bf0d02519c86750d2da33f413e11cf0c6 Yes, it is kind of invalid API use, but since the check is already there, we should make it actually worthwile. >>> >>> lol >>> >>> I agree that checking for it is a good idea, obviously, but I wouldn't >>> call it a regression. >>> >> How about rephrasing the first sentence to: "This commit stops using >> pkt->stream_index as index in an AVFormatContext's streams array before >> actually comparing the value with the count of streams in said array." > > Sure, sounds good. > > > Not that I object to having a check. But then why is check_packet() > called so deep and not immediately on entry to the muxer? I guess it is not that deep, but recent factorization efforts hidden it a bit. >>> >>> You can see in my original commit it is the very first thing done after >>> entering the muxer. Right now it's several function calls deep. >>> >> I could make it the very first thing called in write_packets_common(). > > Why not move the check_packet() call out of prepare_packet() into > av_[interleaved_]write_frame() instead? > This would add code duplication and IMO it is nicer to only check a packet for validity if there is actually a packet to check. - Andreas ___ 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".
Re: [FFmpeg-devel] [PATCH] avformat/mux: Check pkt->stream_index before using it
Quoting Andreas Rheinhardt (2020-05-11 23:58:47) > Anton Khirnov: > > Quoting Andreas Rheinhardt (2020-05-10 21:35:54) > >> Anton Khirnov: > >>> Quoting Marton Balint (2020-05-10 19:45:04) > > > On Sun, 10 May 2020, Anton Khirnov wrote: > > > Quoting Andreas Rheinhardt (2020-05-08 00:55:00) > >> This commit fixes two recent regressions both of which are about using > >> pkt->stream_index as index in an AVFormatContext's streams array before > >> actually comparing the value with the count of streams in said array. > >> 96e5e6abb9851d7a26ba21703955d5826ac857c0 did this in > >> prepare_input_packet() and 64063512227c4c87a7d16a1076481dc6baf19841 did > >> likewise in write_packets_common(). > >> > >> Signed-off-by: Andreas Rheinhardt > >> --- > >> The same error in the same file applied on the same day by two > >> different > >> people. How unlikely. > > > > How is it a regression? Isn't it rather invalid API use? > > Fun fact: 7b03b65bf0d02519c86750d2da33f413e11cf0c6 > > Yes, it is kind of invalid API use, but since the check is already > there, > we should make it actually worthwile. > >>> > >>> lol > >>> > >>> I agree that checking for it is a good idea, obviously, but I wouldn't > >>> call it a regression. > >>> > >> How about rephrasing the first sentence to: "This commit stops using > >> pkt->stream_index as index in an AVFormatContext's streams array before > >> actually comparing the value with the count of streams in said array." > > > > Sure, sounds good. > > > > > > > Not that I object to having a check. But then why is check_packet() > > called so deep and not immediately on entry to the muxer? > > I guess it is not that deep, but recent factorization efforts hidden it > a > bit. > >>> > >>> You can see in my original commit it is the very first thing done after > >>> entering the muxer. Right now it's several function calls deep. > >>> > >> I could make it the very first thing called in write_packets_common(). > > > > Why not move the check_packet() call out of prepare_packet() into > > av_[interleaved_]write_frame() instead? > > > This would add code duplication and IMO it is nicer to only check a > packet for validity if there is actually a packet to check. Okay. I don't entirely agree, but this is not important enough to waste time arguing about. Feel free to push. -- Anton Khirnov ___ 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".
