| Message ID | 20160906162659.GR4692@nb4 |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, Sep 06, 2016 at 06:26:59PM +0200, Michael Niedermayer wrote: > On Tue, Sep 06, 2016 at 05:45:46PM +0200, Hendrik Leppkes wrote: > > On Tue, Sep 6, 2016 at 5:10 PM, Michael Niedermayer > > <michael@niedermayer.cc> wrote: > > > On Tue, Sep 06, 2016 at 02:39:11PM +0200, Hendrik Leppkes wrote: > > >> On Tue, Sep 6, 2016 at 2:21 PM, Michael Niedermayer > > >> <michael@niedermayer.cc> wrote: > > >> > On Tue, Sep 06, 2016 at 02:18:35PM +0200, Michael Niedermayer wrote: > > >> >> On Tue, Sep 06, 2016 at 04:57:06AM +0200, Michael Niedermayer wrote: > > >> >> > On Mon, Sep 05, 2016 at 10:04:35PM -0300, James Almer wrote: > > >> >> > > On 9/5/2016 12:41 PM, Michael Niedermayer wrote: > > >> >> > > > On Mon, Sep 05, 2016 at 04:41:52PM +0200, Clément Bœsch wrote: > > >> >> > > >> From: Clément Bœsch <clement@stupeflix.com> > > >> >> > > >> > > >> >> > > >> These adjusted codec fields do not seem to be in use anymore and prevent > > >> >> > > >> the convert of ffmpeg*.c to codecpar. > > >> >> > > > > > >> >> > > > ./ffmpeg -i ~/tickets/4914/xdcam8mp2-1s_small.ts -c:v copy out.mxf > > >> >> > > > fails, no output anymore > > >> >> > > > > > >> >> > > > ./ffmpeg -i matrixbench_mpeg2.mpg -c:v copy -t 1 test.avi > > >> >> > > > the output now has 600fps > > >> >> > > > > >> >> > > Even with this code in place the resulting stream in the avi is reported > > >> >> > > as 100 fps. > > >> >> > > > >> >> > that seems to be a regression since > > >> >> > 6f69f7a8bf6a0d013985578df2ef42ee6b1c7994 > > >> >> > > > >> >> > IIRC the intended timebase is 1/50 for this kind of content > > >> >> > (allowing the support of interlaced and field duplicated content to > > >> >> > appear later) > > >> >> > > > >> >> > > > >> >> > > And with or without the code, the resulting files play the > > >> >> > > same with the players i tried. > > >> >> > > > >> >> > Higher framerates / finer timebases need noticably more space to > > >> >> > be stored in avi, thats not the case for other formats and thats > > >> >> > one reason why avi is treated as a special case. > > >> >> > > > >> >> > ill try to look tomorrow why its 100fps since the previous > > >> >> > codecpar patches. Though 100fps is not nearly as bad as 600fps > > >> >> > 600 has ~6 times the overhead > > >> >> > > >> >> This regression is caused by ticks_per_frame beiing incorrect > > >> >> > > >> >> Ill send a patch to fix this > > >> > > > >> > patch attached > > >> > > > >> > > >> We don't have time_base in codecpar, so why do we need ticks per frame in it? > > > > > > We need both in some form probably > > > For this regression ticks_per_frame ATM is enough. > > > For time_base theres code to copy/access it bypassing codecpar > > > > > > The way it seems to be currently is that there are fields which > > > are needed and either they are > > > in codecpar or > > > theres some tricks to access them from code sheduled to be removed > > > (which will work only as long as the code isnt removed) > > > or things just dont work. > > > > > > > > >> > > >> Which time_base does it modify the interpretation of? The field should > > >> be bundled with that, then. > > > > > > the AVCodecContext one, ticks_per_frame is already in AVCodecContext > > > the AVCodecContext ticks_per_frame though is not exported after codecpar > > > while the codec timebase still is just not vissibly through codecpar > > > > > > > Maybe that part should be fixed then, wherever we export that to > > AVCodecContext, also set its ticks_per_frame properly? > > It just feels bad to export a property here that standing alone > > doesn't mean anything. > > > > So fix the export of ticks_per_frame for AVCodecContext, and if later > > on we decide we really do need time_base in AVCodecParameters, and > > can't fix whatever needs it differently, we can then include both in > > there. > > attached > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Dictatorship naturally arises out of democracy, and the most aggravated > form of tyranny and slavery out of the most extreme liberty. -- Plato > libavformat/utils.c | 4 +++- > tests/ref/fate/copy-trac4914 | 4 ++-- > tests/ref/fate/copy-trac4914-avi | 4 ++-- > 3 files changed, 7 insertions(+), 5 deletions(-) > 2dc146e807cbdbdbca653a22d827920e8c05b3c8 0001-avformat-Export-ticks_per_frame-in-st-codec.patch > From ae128325081392610475b62d0c20165e0cb9536f Mon Sep 17 00:00:00 2001 > From: Michael Niedermayer <michael@niedermayer.cc> > Date: Tue, 6 Sep 2016 18:10:41 +0200 > Subject: [PATCH] avformat: Export ticks_per_frame in st->codec > > Suggested-by: Hendrik Leppkes > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> applied with this we have restored the functionality to prior bug/regression so it should serve better as a reference. [...]
On 9/7/2016 6:14 AM, Michael Niedermayer wrote: >> libavformat/utils.c | 4 +++- >> > tests/ref/fate/copy-trac4914 | 4 ++-- >> > tests/ref/fate/copy-trac4914-avi | 4 ++-- >> > 3 files changed, 7 insertions(+), 5 deletions(-) >> > 2dc146e807cbdbdbca653a22d827920e8c05b3c8 0001-avformat-Export-ticks_per_frame-in-st-codec.patch >> > From ae128325081392610475b62d0c20165e0cb9536f Mon Sep 17 00:00:00 2001 >> > From: Michael Niedermayer <michael@niedermayer.cc> >> > Date: Tue, 6 Sep 2016 18:10:41 +0200 >> > Subject: [PATCH] avformat: Export ticks_per_frame in st->codec >> > >> > Suggested-by: Hendrik Leppkes >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > applied > > with this we have restored the functionality to prior bug/regression > so it should serve better as a reference. Should be backported to the 3.1 branch. Regarding this whole time_base/ticks_per_frame issue, couldn't we maybe add both fields (merged or as is) to codecpar as Clément suggested, but as internal/hack/nonpublic instead at least until we find a proper way to solve the stream copy case? I know adding private-but-not-really fields suck, but so does being stuck here because AVI is a crappy format. Alternatively, since until now ffmpeg.c's stream copy has been using the initialized AVCodecContext from AVStream, can't we alloc, initialize and use a separate one, much like it's done for actual transcoding? Would that be enough for the decoder to set the two fields? I'm throwing shit at the wall to see what sticks, because i barely know half of what this whole problem entails. But i do know that the more we bikeshed the less chances it will be resolved in a timely and adequate manner. For that matter, libav clearly didn't have this issue. Does that means avconv is creating broken files in these specific cases with stream copy?
On Wed, Sep 07, 2016 at 10:07:26PM -0300, James Almer wrote: > On 9/7/2016 6:14 AM, Michael Niedermayer wrote: > >> libavformat/utils.c | 4 +++- > >> > tests/ref/fate/copy-trac4914 | 4 ++-- > >> > tests/ref/fate/copy-trac4914-avi | 4 ++-- > >> > 3 files changed, 7 insertions(+), 5 deletions(-) > >> > 2dc146e807cbdbdbca653a22d827920e8c05b3c8 0001-avformat-Export-ticks_per_frame-in-st-codec.patch > >> > From ae128325081392610475b62d0c20165e0cb9536f Mon Sep 17 00:00:00 2001 > >> > From: Michael Niedermayer <michael@niedermayer.cc> > >> > Date: Tue, 6 Sep 2016 18:10:41 +0200 > >> > Subject: [PATCH] avformat: Export ticks_per_frame in st->codec > >> > > >> > Suggested-by: Hendrik Leppkes > >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > applied > > > > with this we have restored the functionality to prior bug/regression > > so it should serve better as a reference. > > Should be backported to the 3.1 branch. > > Regarding this whole time_base/ticks_per_frame issue, couldn't we maybe > add both fields (merged or as is) to codecpar as Clément suggested, but > as internal/hack/nonpublic instead at least until we find a proper way > to solve the stream copy case? There might be another way. If this is moved to lavf/utils, the code will have access to AVStream->internal->avctx, which is (still) legit. AFAICT, this will require a function such as avformat_remux_copy_timebase(). But is this what we really want? Also, we need to clarify the use of copy_tb. Currently, according to the doc, 0=demuxer, 1=decoder, -1=auto but 2 is not documented (maybe it's time to move the cli options to an AVOption system to have constants). > I know adding private-but-not-really fields suck, but so does being stuck > here because AVI is a crappy format. > This is not only for avi, see eed7e08 cf9500a ba96a2a > Alternatively, since until now ffmpeg.c's stream copy has been using the > initialized AVCodecContext from AVStream, can't we alloc, initialize and > use a separate one, much like it's done for actual transcoding? Would > that be enough for the decoder to set the two fields? And re-do the probe somehow to fill its parameters? > I'm throwing shit at the wall to see what sticks, because i barely know > half of what this whole problem entails. But i do know that the more we > bikeshed the less chances it will be resolved in a timely and adequate > manner. Yes, and this is really hindering any progress with the merge (ETA: ~300 commits, twice as worse as a few weeks ago). > For that matter, libav clearly didn't have this issue. Does that means > avconv is creating broken files in these specific cases with stream copy? AFAIK, yes
On Wed, Sep 07, 2016 at 10:07:26PM -0300, James Almer wrote: > On 9/7/2016 6:14 AM, Michael Niedermayer wrote: > >> libavformat/utils.c | 4 +++- > >> > tests/ref/fate/copy-trac4914 | 4 ++-- > >> > tests/ref/fate/copy-trac4914-avi | 4 ++-- > >> > 3 files changed, 7 insertions(+), 5 deletions(-) > >> > 2dc146e807cbdbdbca653a22d827920e8c05b3c8 0001-avformat-Export-ticks_per_frame-in-st-codec.patch > >> > From ae128325081392610475b62d0c20165e0cb9536f Mon Sep 17 00:00:00 2001 > >> > From: Michael Niedermayer <michael@niedermayer.cc> > >> > Date: Tue, 6 Sep 2016 18:10:41 +0200 > >> > Subject: [PATCH] avformat: Export ticks_per_frame in st->codec > >> > > >> > Suggested-by: Hendrik Leppkes > >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > applied > > > > with this we have restored the functionality to prior bug/regression > > so it should serve better as a reference. > > Should be backported to the 3.1 branch. done locally > > Regarding this whole time_base/ticks_per_frame issue, couldn't we maybe > add both fields (merged or as is) to codecpar as Clément suggested, but > as internal/hack/nonpublic instead at least until we find a proper way > to solve the stream copy case? > I know adding private-but-not-really fields suck, but so does being stuck > here because AVI is a crappy format. > > Alternatively, since until now ffmpeg.c's stream copy has been using the > initialized AVCodecContext from AVStream, can't we alloc, initialize and > use a separate one, much like it's done for actual transcoding? Would > that be enough for the decoder to set the two fields? currently the 2 fields are filled in by avformat_find_stream_info() parsing and or decoding headers and packets and these can require many packets. The 2 fields really arent special, avformat_find_stream_info() fills in alot of little bits and pieces from informative over occasional useful to critical things. What avformat_find_stream_info() does could be moved into applications But that would duplicate the code in many user applications and make it impossible to centrally fix bugs or add features. It could be moved under a different API too of course. This would raise the question of "Why" we again reimplement the API. Theres also a general lack of discussions about design changes and API deprecations on ffmpeg-devel, which may be part of the problem [...]
From ae128325081392610475b62d0c20165e0cb9536f Mon Sep 17 00:00:00 2001 From: Michael Niedermayer <michael@niedermayer.cc> Date: Tue, 6 Sep 2016 18:10:41 +0200 Subject: [PATCH] avformat: Export ticks_per_frame in st->codec Suggested-by: Hendrik Leppkes Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/utils.c | 4 +++- tests/ref/fate/copy-trac4914 | 4 ++-- tests/ref/fate/copy-trac4914-avi | 4 ++-- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/libavformat/utils.c b/libavformat/utils.c index 7d23c4a..76cbff4 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -3825,8 +3825,10 @@ FF_DISABLE_DEPRECATION_WARNINGS st->codec->height = st->internal->avctx->height; } - if (st->codec->codec_tag != MKTAG('t','m','c','d')) + if (st->codec->codec_tag != MKTAG('t','m','c','d')) { st->codec->time_base = st->internal->avctx->time_base; + st->codec->ticks_per_frame = st->internal->avctx->ticks_per_frame; + } st->codec->framerate = st->avg_frame_rate; if (st->internal->avctx->subtitle_header) { diff --git a/tests/ref/fate/copy-trac4914 b/tests/ref/fate/copy-trac4914 index 9301c86..c977f30 100644 --- a/tests/ref/fate/copy-trac4914 +++ b/tests/ref/fate/copy-trac4914 @@ -1,2 +1,2 @@ -84316a64b609052d9974891686fbf607 *tests/data/fate/copy-trac4914.mxf -566329 tests/data/fate/copy-trac4914.mxf +8868ae16d99ed03916e9dc7105285471 *tests/data/fate/copy-trac4914.mxf +560697 tests/data/fate/copy-trac4914.mxf diff --git a/tests/ref/fate/copy-trac4914-avi b/tests/ref/fate/copy-trac4914-avi index e23affc..0ee6675 100644 --- a/tests/ref/fate/copy-trac4914-avi +++ b/tests/ref/fate/copy-trac4914-avi @@ -1,2 +1,2 @@ -e948f10c90f526ae2c0cf234e1f54128 *tests/data/fate/copy-trac4914-avi.avi -480886 tests/data/fate/copy-trac4914-avi.avi +26e4202638bc384b82d2b5eb4d33a5f0 *tests/data/fate/copy-trac4914-avi.avi +479494 tests/data/fate/copy-trac4914-avi.avi -- 2.9.3