[Kdenlive-devel] AVCHD seeking committed into FFmpeg repository

[Dan Dennedy]

I took some time to review the patch.

2009/8/23 Ivan Schreter <schreter at gmx.net>:
> Hi Dan,
>
> Dan Dennedy wrote:
>
> Not sure why this did not get to the MLT list.
>
> 2009/8/22 Ivan Schreter <schreter at gmx.net>:
>
>
> Hi all,
>
> FYI: since there were no objections from FFmpeg since a week, I've just
> committed my seeking patches to FFmpeg repository (revision 19681). MPEG-TS
> (and thus also AVCHD) seeking now works, at least for AVCHD movies from my
> Panasonic camcorder. I know that Canon camcorders had some issues with
> proper key frame detection (files are missing appropriate SEI messages), so
> they might still have a problem.
>
> As for MLT, I've updated my patch for the newest MLT revision (as of today,
> 2009-08-22). I didn't test it too much, though, but here it is attached. So
> feel free to test it.
>
>
> That's great! I will apply the patch, but in a way that requires an
> explicit configure flag to enable it.

I see you set a use_new_seek flag. That is really nice. I see a couple
of changes not guarded by this, and I guess that means they are
general improvements as well - no regression. If there are, it would
not be hard to add some more 'if' protection. Now, instead of a
configure flag, and a static variable, I wonder if it is better to
just add a property with this name to toggle behaviour. That would
make it easier for all to test. Then, when the default changes to
enabled but until the if/else conditions are removed, one could
explicitly disable the new seek behaviour for troubleshooting.

> MPEG-PS movies (e.g., DVD VOB) don't work correctly with MLT patch only. Two
> patches for FFmpeg are still needed, one which introduces proper seeking for
>
>
> I think I will name the flag --avformat-break-mpegps to make people
> acknowledge the risk. My clients and I use a lot of non-VOB/DVD MPEG-2
> Program Streams.
>
>
>
> I'm working on another patch for MPEG-PS, so that will be supported in the
> near future correctly as well, I hope.
>
> Since the changes I made are in if() conditions, you can eventually use this

Now I understand what this meant.

> to disable it for MPEG-PS for now. Configure flag with name "*-breaks-*"

And you are suggesting we can make the default value of the flag
conditional based on certain formats. Makes sense - maybe as a step
between default disabled, and defaulting to enabled for all formats.

> doesn't sound particularly good :-)
>
> MPEG-PS, one to fix a bug in mpegvideo parser. These patches are not yet
> accepted by FFmpeg, but I attached them here. They also cause seeking
> regression (patch NOT attached).
>
>
> Please explain further the last line. I had first read it to mean:
> applying the patches breaks seeking on ps when the app has not made
> some adjustments. However, I do not understand what patch you refer to
> when you say it is not attached.
>
>
>
> Only the patch with regression is not attached. The two FFmpeg patches for
> parser fix and seek update were attached, so you have complete code.
>
> Applying the seeking patch on FFmpeg doesn't break apps. It just seeks
> correctly. So the current workaround in MLT with seeking one second backward
> will work with the patch in FFmpeg as well, you'll just get less performance
> (as it is today) and off-by-2-3-frames. Only MLT patch and no FFmpeg patches
> will break seeking of MPEG-PS.
>
> I hope it's clear now...

Yes. I have one more big comment from the review.

You added a seek-to-first-keyframe in producer_open(). However, I want
to avoid seeking there to avoid adding addtional time to just load a
file. When there is a large project file, it can have a big impact on
project load times. There is already a problem in kdenlive with that.
I have a commercial client I support that loads 24 hour playlists of
music videos, short channel promos, advertisements, etc. Imagine that.

If we defer this initial seek to inside producer_get_image, will it
cause a problem if audio is fetched first? To be specific, I would put
it inside that function around line 797, inside the following block:

	// Seek if necessary
	if ( position != expected || last_position == -1 )
...
		else if ( seekable && ( position < expected || position - expected
>= 12 || last_position == -1 ) )
		{
			<here>

The _last_position property is initialized to -1. There is another
place where it is set back to -1, however. So, we can initialize
_last_position to -2, and only do your new logic on -2. (And convert
these values to more meaningful constants.)

I will experiment with a modified version of your patch encompassing
my thoughts.

P.S. I have been giving more thought about your idea to add some frame
caching, and I want to do that before I add a YADIF deinterlacer
because YADIF requires previous, current, and next frames.

-- 
+-DRD-+