[FFmpeg-devel] [PATCH] avformat/mux: Check pkt->stream_index before using it

[Anton Khirnov]

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 <andreas.rheinhardt at gmail.com>
> >>>>>> ---
> >>>>>> 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