GNOME Bugzilla – Bug 586891
[qtdemux] skips all tracks in a media file because of their duration
Last modified: 2013-08-21 18:13:15 UTC
I have a test clip here that does not play with qtdemux. No stream are being demuxed because each trak has a duration that enters the *HACK* if case checking for duration. I don't really understand that code and how it checks the trak duration. I tried rewriting it myself and now the file works, so I might misunderstand the way timescale needs to be applied or the code was wrong. Will attach a patch for review.
Created attachment 137343 [details] [review] Compare trak duration with complete duration correctly.
looks like the hack had a typo bug in the tdur1/tdur2 calculations (those variables could be renamed for easier reading btw). The patch therefore makes sense to me.
Patch committed to GIT. Closing bug.
Reopening, since this causes regressions with other clips for me.
Created attachment 137427 [details] debug log (gzipped)
I have more files that fail to play now that used to play with the last release. I propose to revert this patch until this gets sorted out. Ping me on IRC for sample files.
And reverted. Thanks for the riveting discussion. commit f6a12114953214d65560b084197362e814376d17 Author: Tim-Philipp Müller <tim.muller@collabora.co.uk> Date: Wed Jul 1 09:32:42 2009 +0100 Revert "qtdemux: Make sure we don't blacklist streams by wrongly comparing their" This reverts commit 5503a59a5779b67451d8a271000181790ee76bc7. Reverting this since it causes regressions with a lot of sample files I have, all of which worked fine with the last -good release (#586891).
Created attachment 137680 [details] [review] rewrite code for clarity I am not convinced the patch is correct. As I understand it, timescale is a *divisor*. The multiplication with the 'wrong' timescale (the presumed typo) should work out just fine (ie. like a division by the 'right' timescale values) when doing the durA/durB calculation later on, since (d1/ts1)/(d2/ts2) = (d1*ts2)/(d2*ts1). Bonus points for clarity this doesn't get of course. Attaching a hopefully clearer version of the code. I don't think it changes the logic though. If you could make your sample file available, that would be much appreciated. Edward suggested looking at the index and blacklisting streams with just one chunk instead of looking at the duration. Would that make things work with your file?
Created attachment 138596 [details] [review] Patch adjusting global duration using the tracks and delaying pad adding. This patch tries to fix the global duration at various steps : 1) When timescale is 0 it tries to initialize it with a valid duration from a track. 2) When the global duration is really wrong compared to the longest track it gets adjusted. Introduced a function to destroy a stream which fixes various memleaks when stream creation fails.
Created attachment 139091 [details] [review] Improved patch This patch will fix duration reading depending on the version number. It will also check the number of samples on top of the track duration before discarding it as a image header.
What's the state of this bug?
Reopening as I can't see any open question
And ping again ;) Julien's change makes sense, Tim's change multiplies a factor of GST_SECOND to the durations but other than that makes sense too... can we get both in? Any reason not to?
Is this even still a problem with current qtdemux? > Any reason not to? We're flying blind, ie. don't have a sample file.
No feedback at all, so let's get rid of this bug.