Rework packet finding logic #468
Conversation
|
I guess it could be correct |
dwbuiten
left a comment
dwbuiten
left a comment
There was a problem hiding this comment.
I assume all the tests pass, and the little cut file I posted previously, all work now?
I'd love to add that as a test but as I am no longer working for by the company which runs the GCS bucket that hosts the test files, either someone there will have to upload it, or we'll have to move the test files to be hosted somewhere else.
| void DecodeNextFrame(int64_t &PTS, int64_t &Pos); | ||
|
|
||
| // Returns the first packet read that has nonzero PTS/DTS (depending on Frames.UseDTS) | ||
| SmartAVPacket DecodeNextFrame(); |
There was a problem hiding this comment.
A decode function returning a packet (and not even the one it decoded) seems extremely confusing to read and follow through the code. Preferably it would actually fill the "unique packet index" or hash or whatever in a function argument, which would negate the need for all this smart packet stuff in the first place, and, at least in my opinion, be easier to follow. YMMV though, that is just my personal take/preference.
There was a problem hiding this comment.
I guess FindPacket could be called directly in DecodeNextFrame() and return its result through an output parameter, yes. (This would be slightly slower since it'd call FindPacket() more often, but that's probably negligible here seeing as we are working with video.)
With the current logic, this would mean that DecodeNextFrame() would need two output arguments AStartTime and FirstPacketIndex, though, seeing as so far I preserved the logic in GetFrame() that deals with StartTime. (I have not yet run into a case where it triggers, but I wanted to change as little behavior as possible.)
Personally, I prefer the current logic because it has simpler control flow, even if the semantics of the DecodeNextFrame() function are a bit unfortunate. But if you prefer the other option I can change this and drop all of the SmartAVPacket logic.
|
|
||
| // RAII wrapper for AVPacket * that handles av_packet_alloc and av_packet_free. | ||
| // av_packet_ref and av_packet_unref still need to be handled by user code. | ||
| class SmartAVPacket : public std::unique_ptr<AVPacket, void(*)(AVPacket *)> { |
There was a problem hiding this comment.
Is everywhere this used actually exception safe? I ask because now there is no longer an explicit alloc failure check, and the unfortunate "exceptions as flow control" history of FFMS (and C++ in general) comes into play.
There was a problem hiding this comment.
It is at the moment because every instance of av_packet_alloc that was replaced had a similar check for the resulting pointer being null and would throw the same FFMS_Exception if it was, so this does not change any exception behavior.
This is why I moved the failure check into the constructor to cut down on some boilerplate, but I see how this is now less explicit about control flow in the case of allocation failure. If you prefer being more explicit about where these exceptions are thrown, I can revert this.
|
Yes, the tests pass and I verified that #469 does not simply hide the tests failure - rebasing the incorrect #457 on top of #469 still makes the tests fail as it should, but this PR on top of #469 passes the tests, fixes the sample from #456, and does not cause any regressions on my sample collection. |
This is in preparation for DecodeNextFrame returning an AVPacket, at which point a RAII wrapper may be helpful to better track ownership semantics. This uses a simple std::unique_ptr alias that handles av_packet_alloc and av_packet_free, while av_packet_ref and av_packet_unref are still handled manually.
Instead of writing the pts and pos to output args. This does slightly change behavior: Previously, DecodeNextFrame would return - the pos of the first packet read, and - the first *nonnegative* pts (or dts if Frames.UseDTS is true) of a packet read. Hence, AStartTime and Pos could potentially belong to different packets if the first packet read had a negative PTS, like in an mp4 whose start is cut off by an edit list. In such cases, returning the first nonnegative PTS is the necessary with the current logic in GetFrame (which excludes hidden frames in the FrameFromPTS call), so keep this logic for now. This will change in the next commit, but this way this intermediate commit won't break any test files.
After seeking, GetFrame() tries to find out where the demuxer ended up after the seek by trying to recognize the first packet that comes out of the demuxer afterwards by its PTS (or DTS if Frames.UseDTS is true), or by its Pos if that fails. Similarly, DecodePacket() needs to recognize the packet to see if it's a second field of some interlaced frame. When the same PTS can appear multiple times in the same file (e.g. in files with edit lists), this can cause issues. This commit replaces the FrameFromPTS / FrameFromPos functions and the fallback logic in GetFrame() by a single FindPacket function that takes an entire AVPacket. FindPacket() tries to find the unique packet in the index whose TS/Pos/Flags match the given packet. If no such packet exists, it tries a fallback sequence of comparing fewer fields. The current fallback sequence is mostly chosen based on intuition - for well-behaved files simply checking all fields should work fine. If examples come up where this fallback sequence becomes relevant, it can be adjusted.
arch1t3cht
force-pushed
the
packet_finding_rework
branch
from
3c425f0 to
9c943eb
Compare
3 participants
Implement the logic suggested in #457 (comment) .
Supersedes #457 and #456,