Created attachment 363884 [details] [review]
nvdec: implement flush/drain

Comment on attachment 363884 [details] [review]
nvdec: implement flush/drain

In the first case (flush) you'd like to get all frames discarded ASAP, while in the second all pending frames should go downstream.

Is this correctly handled, is it possible to do the flushing faster without trying to push frames downstream first? Note also that pushing a frame downstream with return FLUSHING, which might stop it from outputting (and thus discarding) any further frames?

(In reply to Sebastian Dröge (slomo) from comment #2)
> In the first case (flush) you'd like to get all frames discarded ASAP, while
> in the second all pending frames should go downstream.

There is no 'discard inflight frames' in the API so we can't differentiate those two cases to the library.

> Is this correctly handled, is it possible to do the flushing faster without
> trying to push frames downstream first?

You mean calling release_frame() instead of finish_frame() ?  Otherwise, no.

> Note also that pushing a frame
> downstream with return FLUSHING, which might stop it from outputting (and
> thus discarding) any further frames?

Huh? Which case are we talking about now?

The entire decode sequence is synchronous and there is only ever one frame in flight between decode and display at a time so this is currently not a problem.

In any case, attempting to drop or release frames in flush also breaks lipsync in the same way.

There are not multiple frames in flight if there are B frames?

The case I was thinking of is that during flushing, downstream will return GST_FLOW_FLUSHING. If there were multiple frames in flight, this would potentially cause only the first to be tried to be pushed, and the others to be kept around still.

(In reply to Sebastian Dröge (slomo) from comment #4)
> There are not multiple frames in flight if there are B frames?

There's only 0, or 1 frames in flight between decode and display regardless of B frames which is where this matters in nvdec's frame handling.  Any frames that are subsequently parsed will not find a corresponding GstVideoCodecFrame (because videodecoder clears the list of frames) and will thus be dropped automatically.

> The case I was thinking of is that during flushing, downstream will return
> GST_FLOW_FLUSHING. If there were multiple frames in flight, this would
> potentially cause only the first to be tried to be pushed, and the others to
> be kept around still.

See above.

commit 4e422bec4fa7be03f7540c2c1d939b36305d0fb3
Author: Matthew Waters <matthew@centricular.com>
Date:   Fri Nov 17 17:09:22 2017 +1100

    nvdec: implement flush/drain
    
    Fixes outputted frame sequence when performing a seek
    
    i.e. when seeking backwards, the first frame after the seek was a frame
    from the future.  This would result in GstVideoDecoder essentially
    marking all the timestamps as essentially bogus and the base class would
    attempt to compensate.  A visible indication of this was 'decreasing timestamp'
    warning after a seek.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=790478

It's strange that both your flush and drain implementation are so similar. It gives me the impression that you push the remaining frames on flush, which is wrong, you should drop them.

See above, but pushing them on flush *will* drop them ;)