videosource: Prevent SeekTo() from targeting negative frame numbers

[rzumer]

This prevents an exception from being thrown if the source's first frame is marked as skipped. The initial seek call ignores this, but 6467a17 forces a reseek and calls FFMS_Track::FrameFromPTS() with AllowHidden set to false. This leads it to return -1, and GetFrame() then decreases SeekOffset to -10. When SeekTo() is called again, the offset points it to a negative frame number, and it throws the exception. By clamping TargetFrame in SeekTo() to [0, n + SeekOffset, ], subsequent seek calls can target the first frame or keyframe again and continue decoding normally.

(Can't replace std::min() and std::max() with std::clamp() because the CI runner doesn't support C++17 it seems.)

[dwbuiten]

Tagging @arch1t3cht in this since I know you have a big regression suite you run (and I think you're the original author).

[moi15moi]

CI runner doesn't support C++17 it seems.

No, its just autotools that use c++11

ffms2/Makefile.am

Line 19 in 6601f8c

AM_CXXFLAGS = -std=c++11 -fvisibility=hidden

[arch1t3cht]

Do you have an example file triggering this? That'd make it easier to find out where exactly the current logic is going wrong.

It's hard to know for sure without an example file, but to me it feels like this should be fixed elsewhere instead, like for example in the FrameFromPTS call. The SeekOffset logic should trigger once we don't know where our seek put us in the container, but in this case we do know - we got back a frame that we recognize, it's just marked as hidden.

It's been over a year so I don't remember all the details, but the AllowHidden parameter was initially added in 01cae89 to be able to include hidden frames in the FrameFromPTS call in DecodePacket but keep its behavior elsewhere (like in GetFrame) the same. Back then, I wrote:

As far as I can see, the added AllowHidden arguments for
FrameFromPos and FrameFromPTS are not actually necessary, and can just
always be true, but this way less existing behavior is changed. This
can be revisited when a sample comes up where this would actually
make a difference (i.e. the first frame after seeking being hidden,
or ending up somewhere unknown after a seek in a file with hidden
frames).

"The first frame after seeking being hidden" seems to be the case we're running into here, so it may be time to look into whether AllowHidden can just be set to true everywhere. I'll run some regression tests for that when I can, but either way, a sample file for this exception would be very helpful.

[dwbuiten]

@arch1t3cht here is a 10mb cut (simple dd if=sample.mp4 of=repro.cut.mp4 bs=1M count=10) of one of the samples that can repro this (zip file since github is silly): repro.cut.zip

[arch1t3cht]

Thanks. This is indeed fixed by also including hidden frames in GetFrameFromPTS/Pos everywhere in GetFrame, and I confirmed that this doesn't cause any regressions on my set of samples, so I'd propose to fix this that way instead. I'll send a PR in a second.

[rzumer]

I know it's not ideal because it still decreases the seek offset and keeps it there, I just went with the solution that I found least likely to cause side effects, and I think it makes sense to set a minimum TargetFrame in SeekTo() considering it already clamps it down to the last frame if n + SeekOffset is beyond that anyway.