diff mbox

[FFmpeg-devel] vf_fps: Don't flush a cached frame if it should have been dropped

Message ID 20161004205845.GJ5270@nb4
State Not Applicable
Headers show

Commit Message

Michael Niedermayer Oct. 4, 2016, 8:58 p.m. UTC
On Tue, Oct 04, 2016 at 04:21:29PM -0400, Derek Buitenhuis wrote:
> This fixes downconverting framerates to multiples.
> 
> For example, prior to this patch, converting 900 frames at 60 fps
> to 30 fps would output 451 frames instead of the correct 450.
> 
> Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
> ---
> DISCLAIMER: I don't know libavfilter very well, and I am not
> sure this is the correct fix to the problem. Comments definitely
> welcome.
> 
> Also, I would be happy if any replies could be CC'd to me.

breaks fate
and the change to fate looks wrong, the last frame is lost
make fate-filter-fps
TEST    filter-fps
Test filter-fps failed. Look at tests/data/fate/filter-fps.err for details.

[...]

Comments

Derek Buitenhuis Oct. 4, 2016, 9:13 p.m. UTC | #1
On 10/4/2016 9:58 PM, Michael Niedermayer wrote:
> breaks fate
> and the change to fate looks wrong, the last frame is lost
> make fate-filter-fps
> TEST    filter-fps
> --- ./tests/ref/fate/filter-fps 2016-10-04 14:46:19.642736770 +0200
> +++ tests/data/fate/filter-fps  2016-10-04 22:54:01.859353244 +0200
> @@ -84,4 +84,3 @@
>  0,         78,         78,        1,    30576, 0xa2fcd06f
>  0,         79,         79,        1,    30576, 0xa2fcd06f
>  0,         80,         80,        1,    30576, 0xa2fcd06f
> -0,         81,         81,        1,    30576, 0xd4150aad
> Test filter-fps failed. Look at tests/data/fate/filter-fps.err for details.

This "break" may actually be more correct output, but I am not familiar enough
with lavfi or vf_fps to say. Removing the last frame in case where it should have
been removed was the entire point of this patch.

Perhaps someone more familiar with the code here can comment? Nicholas, maybe?

- Derek
Michael Niedermayer Oct. 4, 2016, 9:21 p.m. UTC | #2
On Tue, Oct 04, 2016 at 10:13:41PM +0100, Derek Buitenhuis wrote:
> On 10/4/2016 9:58 PM, Michael Niedermayer wrote:
> > breaks fate
> > and the change to fate looks wrong, the last frame is lost
> > make fate-filter-fps
> > TEST    filter-fps
> > --- ./tests/ref/fate/filter-fps 2016-10-04 14:46:19.642736770 +0200
> > +++ tests/data/fate/filter-fps  2016-10-04 22:54:01.859353244 +0200
> > @@ -84,4 +84,3 @@
> >  0,         78,         78,        1,    30576, 0xa2fcd06f
> >  0,         79,         79,        1,    30576, 0xa2fcd06f
> >  0,         80,         80,        1,    30576, 0xa2fcd06f
> > -0,         81,         81,        1,    30576, 0xd4150aad
> > Test filter-fps failed. Look at tests/data/fate/filter-fps.err for details.
> 
> This "break" may actually be more correct output, but I am not familiar enough
> with lavfi or vf_fps to say. Removing the last frame in case where it should have
> been removed was the entire point of this patch.

the change to fate is wrong
the input has a timebase of 1/3000, all timestamps are multiplies of
100 its thus basically 30 fps (with skiped frames) and the used
-vf fps=30 should restore skip frames to make it cfr, loosing the last
frame entirely is wrong in this case

codec copy:
#tb 0: 1/3000
...
0,          0,          0,      600,     4287, 0xa259fe7b
0,        600,        600,      700,     3951, 0x1bfc9daf, F=0x0
0,       1300,       1300,     6000,     4232, 0x75aeff18, F=0x0
0,       7300,       7300,      800,     4124, 0xc244436a, F=0x0
0,       8100,       8100,      695,     4104, 0x24aa4d34, F=0x0


without fps=30
0,          0,          0,        1,    30576, 0xcdc29b3d
0,          6,          6,        1,    30576, 0x5c83656c
0,         13,         13,        1,    30576, 0x26b67f83
0,         73,         73,        1,    30576, 0xa2fcd06f
0,         81,         81,        1,    30576, 0xd4150aad


[...]
Derek Buitenhuis Oct. 4, 2016, 9:35 p.m. UTC | #3
On 10/4/2016 10:21 PM, Michael Niedermayer wrote:
>> This "break" may actually be more correct output, but I am not familiar enough
>> with lavfi or vf_fps to say. Removing the last frame in case where it should have
>> been removed was the entire point of this patch.
> 
> the change to fate is wrong
> the input has a timebase of 1/3000, all timestamps are multiplies of
> 100 its thus basically 30 fps (with skiped frames) and the used
> -vf fps=30 should restore skip frames to make it cfr, loosing the last
> frame entirely is wrong in this case

I think you are correct. However, I'm not exactly sure of the proper way to fix
this. Not without more thought...

- Derek
Nicolas George Oct. 6, 2016, 5:37 p.m. UTC | #4
Le tridi 13 vendémiaire, an CCXXV, Derek Buitenhuis a écrit :
> This "break" may actually be more correct output, but I am not familiar enough
> with lavfi or vf_fps to say. Removing the last frame in case where it should have
> been removed was the entire point of this patch.
> 
> Perhaps someone more familiar with the code here can comment? Nicholas, maybe?

If this is referring to me, sorry, the only thing I know about the fps
filter is that it is in need of a good cleanup. Hopefully, the patch series
I am struggling with right now will make it easier.

Regards,
diff mbox

Patch

--- ./tests/ref/fate/filter-fps 2016-10-04 14:46:19.642736770 +0200
+++ tests/data/fate/filter-fps  2016-10-04 22:54:01.859353244 +0200
@@ -84,4 +84,3 @@ 
 0,         78,         78,        1,    30576, 0xa2fcd06f
 0,         79,         79,        1,    30576, 0xa2fcd06f
 0,         80,         80,        1,    30576, 0xa2fcd06f
-0,         81,         81,        1,    30576, 0xd4150aad