GNOME Bugzilla – Bug 785237
omxvideodec: Fix segment seek
Last modified: 2017-07-25 14:10:21 UTC
The segment seek stalled since the decoder state was not reset after drain. It's a bit like GStreamer, you need to reset after sending the decoder an EOS signal, otherwise it rejects incoming buffers. This was tested on Tegra OMX.
Created attachment 356127 [details] [review] omxvideodec: Fix segment seek On segment seek, unlike EOS, we drain, but we cannot expect a flush later to reset the decoder state. As a side effect, the decoder would remain in EOS state and ignore any new incoming buffers. To fix this, we call _flush() inside the _drain() function, and _finish() becomes what _drain() was before. This way, for _finish() (the eos case) we only drain, for _drain() triggered by segment seek or new caps, we also reset the decoder state so it's ready to accept buffers.
Review of attachment 356127 [details] [review]: Looks good!
Attachment 356127 [details] pushed as 64f7f78 - omxvideodec: Fix segment seek
Is it applicable to gstomxaudiodec.c as well ?
(In reply to Julien Isorce from comment #4) > Is it applicable to gstomxaudiodec.c as well ? Good question, I was doing segment seek on video only file here, so I didn't notice. Please find my test program here, in case you have a chance to test before me, https://paste.fedoraproject.org/paste/1vi9Fp9RwshNoHs93UZdoQ The second benefit of doing this is that it prevents buggy component that would drain early and keep pushing old buffer from messing things up (the base class gets really confused in this case).
Actually, on the audio side, the porting to 1.0 went pretty wrong. The base class does not send a NULL buffer anymore on EOS (it used to in 0.10), only the remaining data in the adapter is being pushed and the base class assumed that decoding is synchronous. We should in theory see "still %d frames left after draining" warning pretty often unless lucky. This should be filed seperatly, as EOS is also affected here. We might need to go over all the audio decoder in fact.
Sorry, miss-read the code, it does send an empty buffer to signal the drain, we just need to figure-out if it works for segment seeks or not.
I have not got a chance to test with audio yet but I have a question. Your change is in the block "needs_disable && is_format_change". So your video contains a resolution change at some point ? Otherwise can you explain why seeking to 0 causes to have both needs_disable and is_format_change to be true ? (could it be because your element set some "hacks" field in gstomx.conf ?) Thx!
(In reply to Julien Isorce from comment #8) > I have not got a chance to test with audio yet but I have a question. Your > change is in the block "needs_disable && is_format_change". So your video > contains a resolution change at some point ? I'm not sure why you say that. No, just a video being looped, single resolution. > Otherwise can you explain why > seeking to 0 causes to have both needs_disable and is_format_change to be > true ? (could it be because your element set some "hacks" field in > gstomx.conf ?) Thx! I'm not sure how to correlate this comment against my patch really. All I do is flush after drain to reset the decoder state (otherwise it's stuck in EOS). Wrong bug # ?
Oki I see it is just a collateral damage that you changed that other block. There is no ->drain and ->finish for the audio base classes so it does not look to be applicable to gstomxaudiodec.c . So I do not plan further digging regarding this issue. Thx!