[8350] breaks mpeg2 decoding during frame advance

[bjm <bjm@…>]

To reproduce:

• Install SVN [8350] or later

• Record a file, or view a live buffer, with an ivtv hardward encoder card.

• Begin playback and find a scene with some continous motion.

• Press Pause.

• Press ">" to advance one frame at a time.

The first advance appears to jump back several frames. Keypresses after that apply fragments of images and decode errors to the initial image. The frontend prints messages like this:

[mpeg2video @ 0xb75c0270]Warning MVs not available
[mpeg2video @ 0xb75c0270]00 motion_type at 13 11
[mpeg2video @ 0xb75c0270]00 motion_type at 28 12
[mpeg2video @ 0xb75c0270]00 motion_type at 10 13
[mpeg2video @ 0xb75c0270]00 motion_type at 4 15
[mpeg2video @ 0xb75c0270]00 motion_type at 10 15
[mpeg2video @ 0xb75c0270]00 motion_type at 2 16
[mpeg2video @ 0xb75c0270]00 motion_type at 7 17
[mpeg2video @ 0xb75c0270]mb incr damaged
[mpeg2video @ 0xb75c0270]00 motion_type at 23 19

Mpeg4 software encoded files advance one frame at a time properly as do mpeg2 files played back with mythfrontend version [8349] or earlier.

[danielk]

I can reproduce this. I just checked my first hunch, that it was the frame-by-frame seeking in AFD::SeekReset?() since this only appears on no-keyframes. But that wasn't it. I think maybe this is just not flushing the A/V buffers when it must; just like the FF/REW seek was doing before.

[danielk]

Ok, so the problem is that we are placing frames that avlib has not yet released on the available queue. This has always been a problem, but somehow was less problematic before my changes in [8350]. This is done in the SeekReset?() and in ClearAfterSeek?(). ClearAfterSeek?() does have a big fat warning in it about being unsafe. It works when we seek past the next keyframe or backward because we don't need past frames when we do that, but if we seek forward within the GOP the only thing making things work before was that buffers are fetched from VideoBuffers? is in FIFO fashion. When on occasion this didn't work we got glitches even before these latest changes; By placing the buffers on the available queue in a slightly different order [8350] has made this occur 90% of the time rather than 10% of the time.

The solution is to track whether avlib is done with the frame, and not place it on the available queue until avlib really is done with it. I'll start working on the fix on Tuesday...

[Isaac Richards]

No, the problem is that [8350] has broken the logic of how it decides to flush libavcodec or not. It used to flush only when necessary. After [8350], it flushes almost every time.

[danielk]

The old logic did not flush when necessary, that was the problem [8350] fixed.

Now it may be flushing when it isn't strictly needed, but I was trying to figure out why this was causing a problem when we seek to the previous keyframe and then step to the frame we want. That should work, even if we flush out our previous decoding of that very same keyframe. It may not be super efficient, but it should work.

I'll fix any extra flushing first though; it shoudn't be too difficult.

[Isaac Richards]

When you call avcodec_flush_buffers, it doesn't just flush the buffers, it resets the entire decoder state, even parsing. This simply cannot be called after a seek, unless we're decoding a keyframe next.

Part of the issue also could be the change to the 'skipframes' loop. For exact seeking, we need to actually decode those frames so it can successfully decode and display something in the middle of a gop, and that loop looks like it is no longer doing so.

The change to AVFD::Reset(bool, bool)'s call of SeekReset? to flush frames is also likely wrong - that's called after what should be a seamless file change, so it certainly shouldn't be flushing the decoder. I don't think the HandleStreamChange? callback should be flushing out the video frames, either.

[danielk]

(In [8399]) References #868.

This is a temporary fix while I work on a more efficient and elegant one. This fixes the skipframe loop and forces a seek on FF.

The forced seek is the temporary part. Fixing the frame discard will take me a little time and this should allow people to use single frame seeking in the meantime.

[bjm <bjm@…>]

First, there is a typo in my test case. It should say to press "->" or Right Arrow to advance one frame at time. Greater than ">" advances one second and is working properly.

For mpeg2, I can verify that with the temporary patch, frame advance does advance and decode frames properly although more slowly and unevenly than in the past when the key is held down.

However, frame advance doesn't move at all for software encoded mpeg4 as of [8399].

2005-12-27 15:56:51.873 Dec: DoFastForward(16141 (16141), do discard frames)
2005-12-27 15:56:52.110 NVP: ClearAfterSeek()
2005-12-27 15:56:52.110 VideoOutputXv: ClearAfterSeek()
2005-12-27 15:56:52.110 VideoBuffers::DiscardFrames(): AAAAAAAAAAAAAAAAAAAAAAAAAAUUAAA
2005-12-27 15:56:52.111 VideoBuffers::DiscardFrames(): AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA -- done()
2005-12-27 15:56:52.798 Dec: DoFastForward(16141 (16141), do discard frames)
2005-12-27 15:56:53.025 NVP: ClearAfterSeek()
2005-12-27 15:56:53.025 VideoOutputXv: ClearAfterSeek()
2005-12-27 15:56:53.025 VideoBuffers::DiscardFrames(): AAAAAAAAAAAAAAAAAAAAAAAAAAAAUUA
2005-12-27 15:56:53.026 VideoBuffers::DiscardFrames(): AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA -- done()
2005-12-27 15:56:53.634 Dec: DoFastForward(16141 (16141), do discard frames)
2005-12-27 15:56:53.875 NVP: ClearAfterSeek()
2005-12-27 15:56:53.875 VideoOutputXv: ClearAfterSeek()
2005-12-27 15:56:53.875 VideoBuffers::DiscardFrames(): UAAAAAAAAAAAAAAAAAAAAAAAAAAAAAU
2005-12-27 15:56:53.875 VideoBuffers::DiscardFrames(): AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA -- done()

Note the desiredFrame and framesPlayed is 16141 for all three key presses. The picture appears frozen though I suppose it has redrawn the same frame three times.

[danielk]

fixes mpeg4 NUV problem, and removes seek hack

[danielk]

Bruce, I realized you meant the arrow key. The attached patch should fix your problems and some others to boot.

I want to get my PVR-350 output working to test that too...

There are two problems I know of with this patch:

1/ frame-by-frame advance on NUV files doesn't advance the first time you press it. 2/ seamless switching ringbuffer doesn't always work with avformatdecoder decoding.

I could work around no. 1 easily enough, but I'd like to figure out the underlying cause. No. 2 may be an older problem, someone was reporting something like this with a PVR-250 without proper logs before, being able to reproduce this may be a good thing...

[danielk]

updated patch

[danielk]

FYI The PVR-350 does not function in keyframe or half second advance, with or without the changes in [8350]; it plays the the video from the last keyframe at normal speed, and doesn't advance properly. It does work in frame-by-frame advance and for larger jumps.

Fixed in v2 of the patch is the exit from FF/REW mode. This would jump to a semi-random location in the file if a newly decoded frame wasn't available and it had to use the scratch frame for the pause frame. This predates [8350], but needed fixing.

Also fixed is the first backward seek in NUV files after a pause during playback, again this problem predates [8350], but needed fixing.

Also fixed is seeking in non-PVR-x50 recordings, broken by [8414].

Problem #1 from my previous note appears to effect all decoders. I need to generate a test video to figure out where the off-by-one error is...

[danielk]

(In [8423]) References #868.

Fixes the mpeg2 single frame advance, without the seek hack.

[danielk]

(In [8425]) References #868.

Takes advantage of our new found knowledge of when a frame is in use by avlib for regular mpeg2 decoding. It puts these frames on the end of the available list after a ReleaseFrames?() call and tries to avoid them if other frames are available when GetNextFreeFrame?() is called. This should prevent frames from getting reused while avlib is still using them in a low buffer situations.

The GetStatus?() debugging rutine now uses caps for frames not thought to be in use by the decoder, and lower case frames marked as in use.

[danielk]

(In [8426]) References #868. References #574.

Fixes a bug with libmpeg2 decoding where we would report a frame as available for viewing before we got a STATE_SLICE with a display_fbuf. This would result in very blocky output after a seek to a keyframe. This fixes the blockyness for low-res PS streams, and reduces it for high-res TS stream. This also releases frames when libmpeg2 is done with them, which should mean less wackyness in low buffer situations (see [8425].)

This bug was documented as a possible problem in the libmpeg2 decoder loop. But it doesn't cause problems during normal playback, only when seeking to I frames, or frames that come shortly after them.

[danielk]

(In [8427]) References #868.

Adds a DiscardFrames?() call in SeekReset?. DeLimbos? avlib decoded frames when avlib is done with then DeLimbos? real NUV frames as soon as we're done decoding onto them.

The DeLimbos? prevent us from decoding onto in use frames with MPEG4 NUVs, and speeds up GetNextFreeFrame?() for real NUVs. The DiscardFrames? speeds up exact seeking in a low buffer situation.

[danielk]

updated to apply to latest svn [8427].

[Jean-Michel <je at n-michel.net>]

svn8424 broke compile on amd64

[danielk]

(In [8447]) References #868. Non-functional changes to AvFormatDecoder?.

Makes SeekReset? VERBOSE macro more informative, and renames some local variables to be more informative of their actual function.

[danielk]

(In [8448]) References #868. Makes most DiscardFrames?() calls safe.

The old VideoBuffers::DiscardFrames?() could only be called safely if the next frame decoded was a keyframe. This adds a parameter to the function 'next_frame_keyframe' if this is false only frames not in limbo are discarded and the list of frames in use by the decoder is not cleared. If 'next_frame_keyframe' is true the old DiscardFrames?() is used.

There are only three remaining places where DiscardFrames?() is called unsafely, one is when the prebuffer wait has timed out 100 times, and the other is when GetNextFreeFrame?() is called with 'allow_unsafe' set to true and there are no frames available for decoding onto. Both of these are designed to prevent deadlock in some pretty dreadful error states so I think it is best to leave these as is. The final case is with XvMC, this is needed just because there are not enough frames available in the nVidia XvMC implementation to handle all seek in all cases. I would recomend not attempting to edit recordings with XvMC enabled anyway.

What this commit means in practice is fewer 'timed out' messages and fewer 'GetNextFreeFrames?() returned busy frame, discarding' messages. Which means some seeks will be a little faster, and those bits of code where DiscardFrames?() needs to be called unsafely is reduced. This in turn means you'll be less likely to see distorted frames after a seek.

[danielk]

(In [8449]) References #868.

Fixes a problem where exiting FF/REW >5x would sometimes return place you in some random part of the file when using an X11 output method other than XvMC. This was due to using the scratch frame on pause without setting it's frameNumber to a reasonable value.

This also disables some very verbose XvMC debugging when VB_PLAYBACK is enabled.

[danielk]

(In [8450]) References #868, non-functional documentation commit.

This just adds some documentation to three methods in the NVP.

[danielk]

(In [8455]) Closes #868.

This is commit is a bit of left over changes from the patch. This refactors DoRewindDVD() and DoRewindNormal?() into a single method DoRewindSeek?().

Also there is a small hack in DoFastForward?() for MPEG4 NUV files. There is an off by one error somewhere in NUV seeking, this causes the frame-by-frame seek to get stuck in a loop sometimes (one frame in every couple hundred it seems.) This hack prevents it from getting stuck in that loop by simply ignoring one frame rewinds in DoFastForward?(). This is safe because DoRewind?() is only called from DoFastForward?() when there is an off-by-one error, in which case we want the hack, and when we were decoding normally before the call, in which case this hack is not triggered because the decoder is more than one frame ahead of the seek position.

There are still problems with seamless ringbuffer switching, but it seems unrelated to the problems solved in this ticket or caused by [8350].