Martin Storsjö: > On Wed, 30 Sep 2020, Andreas Rheinhardt wrote: > >> There are two possible kinds of timecode tracks (with tag "tmcd") in the >> mov muxer: Tracks created internally by the muxer and timecode tracks >> sent by the user. If any of the latter exists, the former are >> deactivated. The former all belong to another track, the source >> track; the latter don't have a source track set, but the index of the >> source track is initially zeroed by av_mallocz_array(). > > Would it be worthwhile to explicitly initialize these to e.g. -1, to > make src_track clearer? >
I wouldn't object to this, but this could only be done after fixing the second point below. (Also note that there is actually an overflow problem with nb_streams when AVFormatContext.nb_streams is huge (it's technically an unsigned, but restricted to the range of int) and if you use -1, you can't really solve the overflow problem by using an unsigned.) >> This is a problem since 3d894db700cc1e360a7a75ab9ac8bf67ac6670a3: Said >> commit added a function that calculates the duration of tracks and the >> duration of timecode tracks is calculated by rescaling the duration >> (calculated by the very same function) of the source track. This gives >> an infinite recursion if the first track (the one that will be treated >> as source track for all timecode tracks) is a timecode track itself, >> leading to a stack overflow. >> >> This commit fixes this by not using the nonexistent source track >> when calculating the duration of timecode tracks not created internally >> by the mov muxer. >> >> Signed-off-by: Andreas Rheinhardt <[email protected]> >> --- >> 1. Remuxing timecode tracks is currently impossible for mp4 (the codec >> tag validation fails); I wonder whether this is actually intended given >> that there is a warning that timecode metadata is ignored if an explicit >> timecode track is present. >> 2. There is another place where the src_track field is used even when a >> timecode track doesn't have a src_track: When writing the moov tag >> (lines 4156-4166). This will probably also need to be fixed, but it is >> not dangerous. >> 3. There is btw no validation that a track with "tmcd" tag is a data >> stream. >> >> libavformat/movenc.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavformat/movenc.c b/libavformat/movenc.c >> index 2006fcee4b..265465f97b 100644 >> --- a/libavformat/movenc.c >> +++ b/libavformat/movenc.c >> @@ -2901,7 +2901,7 @@ static int mov_write_minf_tag(AVFormatContext >> *s, AVIOContext *pb, MOVMuxContext >> >> static int64_t calc_pts_duration(MOVMuxContext *mov, MOVTrack *track) >> { >> - if (track->tag == MKTAG('t','m','c','d')) { >> + if (track->tag == MKTAG('t','m','c','d') && mov->nb_meta_tmcd) { >> // tmcd tracks gets track_duration set in mov_write_moov_tag from >> // another track's duration, while the end_pts may be left at >> zero. > > This fix in itself is probably good and safe, so LGTM. > Thanks, applied. - Andreas _______________________________________________ ffmpeg-devel mailing list [email protected] https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email [email protected] with subject "unsubscribe".
