Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maybe a seeking error after 2.40 #394

Closed
vxzms opened this issue Aug 26, 2021 · 19 comments · Fixed by #402
Closed

Maybe a seeking error after 2.40 #394

vxzms opened this issue Aug 26, 2021 · 19 comments · Fixed by #402

Comments

@vxzms
Copy link

vxzms commented Aug 26, 2021

@misakikasumi get a seeking error when using ffms2_api4, I also reproduced this problem in 2.40, 2.23.1 looks fine.

When seeking a video, there is a high probability that the wrong frame will be returned in test.
The test environment is Windows + Vapoursynth / Aegisub + ffms2 / lsmashsource. Here is the test (sample video link):

  1. Dump video to y4m by vspipe dump_y4m.vpy seek-test.y4m
    dump_y4m.vpy: core.ffms2.Source("seek-test.mkv").set_output()
  2. Make a comparison of these outputs at same frame:
    a. core.ffms2.Source("seek-test.mkv")
    b. core.lsmas.LWLibavSource("seek-test.mkv")
    c. core.ffms2.Source("seek-test.y4m")
    d. core.lsmas.LWLibavSource("seek-test.y4m")
    e. Aegisub open seek-test.mkv use the same ffms2 version as VS

In the test, a and e results are always the same.
outputs of b, c and d are always the same.
a and c are likely to be different.

@AkarinVS
Copy link

regression introduced by 13d9f.

AkarinVS added a commit to AkarinVS/L-SMASH-Works that referenced this issue Sep 12, 2021
I think the new soft reset behavior is fairly safe: tested random
seeking on mpeg2, avc and hevc files. And the user can always switch
it off if the new reset behavior turns out to be problematic.

Also tested the case in FFMS/ffms2#394 and confirmed that lsmas is
still unaffected.

Fixes #10.

Signed-off-by: akarin <i@akarin.info>
@dwbuiten
Copy link
Member

Can you be a bit more specific on what types of files this affects? You've only said "a video" - and it definitely isnt all videos, (not e.g. MP4 AFAICT). Is it specific to Matroska? H.264 in Matroska? Blu-ray H.264 in Matroska?

@dwbuiten
Copy link
Member

Also can you test with master, particularily after: 5da8f49

@ghost
Copy link

ghost commented Sep 12, 2021

It is not specific to Matroska. In fact, I originally spot this with several Blu-ray m2ts files. The sample mkv file is actually a m2ts remux.

@AkarinVS
Copy link

AkarinVS commented Sep 13, 2021 via email

@dwbuiten
Copy link
Member

dwbuiten commented Sep 13, 2021

It definitely does not affect all AVC files, at least not via the C API - I've had petabytes of videos run via that + tests (albeit almost totally only MP4s) running though it since that commit when in. I've not tested via the VS plugin, though.

I have a few possible suspicions:

  • It's posible it may not affect MP4 so I didn't notice (all the unit tests are MP4s, and I mostly only send those though).
  • May be specific to the kinds of flags you'd get in a Blu-ray (fields, or fake fields).
  • It's possible FFmpeg's behavior changed since 2018 and made that commit no longer work - particularily if the delay calculation is no longer valid, it could trip the PAFF check.

I am leaning towards the third possibility, since I would have expected this to come up sooner in the previous 3 years since that commit, otherwise. That also means it will be a giant headache to debug...

I will check into it this week - any further info is of course welcome 🙂 .

@vxzms
Copy link
Author

vxzms commented Sep 13, 2021

I used Vapoursynth-R54, ffms3000 and lsmas to reproduce it many times, both avc and hevc, mkv and mp4.

@dwbuiten
Copy link
Member

I definitely suspect an FFmpeg behavior change, then.

@AkarinVS
Copy link

AkarinVS commented Sep 13, 2021 via email

AkarinVS added a commit to AmusementClub/ffms2 that referenced this issue Sep 13, 2021
Workarounds FFMS#394.

Signed-off-by: akarin <i@akarin.info>
@dwbuiten
Copy link
Member

dwbuiten commented Sep 13, 2021

Option 1 is how the tests I generally run are generated, interestingly enough.

Also, thanks for clarifying it is off-by-one, and testing. I'll try and dig into this.

@TbtBI
Copy link

TbtBI commented Sep 13, 2021

It should affect all avc video files. The latest master branch (rev 9525fcd) also exhibits this issue. I wrote a script to test ffms2 against lsmas. https://github.com/AkarinVS/weird-scripts/blob/master/ffms2/issue394.vpy

You can try this build. I have no issues with it. Here quick test between the different version. It would be great if the latest upstream builds fix the regressions.

@dwbuiten
Copy link
Member

dwbuiten commented Sep 13, 2021

@TbtBI that is interesting, since that is a very recent build - long after the supposed source of seeking issues...

I notice the fork of ffmpeg used in that build has: HomeOfAviSynthPlusEvolution/FFmpeg@95dfde1

@TbtBI
Copy link

TbtBI commented Sep 13, 2021

@TbtBI that is interesting, since that is a very recent build - long after the supposed source of seeking issues...

I notice the fork of ffmpeg used in that build has: HomeOfAviSynthPlusEvolution/FFmpeg@95dfde1

There are included patches too in the link of that build. It seems reverted back some commits:
https://forum.doom9.org/showthread.php?p=1879358#post1879358
https://forum.doom9.org/showthread.php?p=1921589#post1921589

@dwbuiten
Copy link
Member

Some of those contain revert, but that big diff contains a bunch of changes that seem to come form nowhere, without context? That's... kind of a mess.

@dwbuiten
Copy link
Member

I think this should fix it - if anyone of @vxzms @misakikasumi @AkarinVS can test it on problematic files.

diff --git a/src/core/videosource.cpp b/src/core/videosource.cpp
index 006099e..f838380 100644
--- a/src/core/videosource.cpp
+++ b/src/core/videosource.cpp
@@ -630,7 +630,7 @@ bool FFMS_VideoSource::DecodePacket(AVPacket *Packet) {
     // H.264 (PAFF) and HEVC can have one field per packet, and decoding delay needs
     // to be adjusted accordingly.
     if (CodecContext->codec_id == AV_CODEC_ID_H264 || CodecContext->codec_id == AV_CODEC_ID_HEVC) {
-        if (!PAFFAdjusted && DelayCounter > Delay && LastDecodedFrame->repeat_pict == 0 && Ret != 0) {
+        if (!PAFFAdjusted && DelayCounter > Delay && LastDecodedFrame->interlaced_frame == 1 && LastDecodedFrame->repeat_pict == 0 && Ret != 0) {
             int OldBFrameDelay = Delay - (CodecContext->thread_count - 1);
             Delay = 1 + OldBFrameDelay * 2 + (CodecContext->thread_count - 1);

Also, do you happen to have any files you know hit this bug that are less copyrighted? I want to add a unit test, but seek-test.mkv is copyrighted, obviously.

@vxzms
Copy link
Author

vxzms commented Sep 15, 2021

Also, do you happen to have any files you know hit this bug that are less copyrighted? I want to add a unit test, but seek-test.mkv is copyrighted, obviously.

https://samples.mplayerhq.hu/MPEG-4/CDR-Dinner_LAN_800k.mp4 also can reproduce when use ffms3000.

After a reminder from TbtBI, I build ffms2 master that looks fine when use vcpkg’s ffmpeg before test your diff. I'm not sure ffmpeg that myrsloik and akarin used, you can wait for them test it if you have patience.

@dwbuiten

Sorry, it’s my mistake, I used the wrong repo to create the wrong binary.

FFSM2 mater branch can reproduce the error, I test your diff, it works fine on my test video.

But I get error when test https://samples.mplayerhq.hu/MPEG-4/MPEGSolution_stuart.mp4.

@ghost
Copy link

ghost commented Sep 15, 2021

After a reminder from TbtBI, I build ffms2 master that looks fine when use vcpkg’s ffmpeg before test your diff. I'm not sure ffmpeg that myrsloik and akarin used, you can wait for them test it if you have patience.

Gosh… Everyone builds his own ffmpeg… Totally messed up… 😿

@vxzms vxzms closed this as completed Feb 13, 2022
@arch1t3cht
Copy link
Contributor

arch1t3cht commented Jun 6, 2022

I also experienced this issue (and know many other people who did), especially with hevc videos encoded with open-gop=1. I tested at least 3 different videos that are showing this bug on the current master, and the proposed change fixed it for all of them.

Also, do you happen to have any files you know hit this bug that are less copyrighted? I want to add a unit test, but seek-test.mkv is copyrighted, obviously.

I made a test video out of Big Buck Bunny 3D, which is licensed under the Creative Commons Attribution License. The subtitle file has different lines synchronized with all the video cuts. In my case (using ffms2 from Aegisub - specifically this fork, with a custom ffms2 subproject), seeking to the end of Line 3 already shows the third frame of the following shot, instead of the last frame of the current shot. Many later lines behave similarly. The proposed patch fixes all of them.

So, is there a chance of getting this fix committed to master?

@dwbuiten
Copy link
Member

dwbuiten commented Jun 7, 2022

OK, I'll look into doing that this week.

dwbuiten added a commit to dwbuiten/ffms2 that referenced this issue Jun 16, 2022
…erlaced

Otherwise seeking can be jank in certain types of streams like open GOP HEVC
or MP4s with huge edit list trims a the very start.

Fixe FFMS#394.

Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
dwbuiten added a commit that referenced this issue Jun 18, 2022
…erlaced

Otherwise seeking can be jank in certain types of streams like open GOP HEVC
or MP4s with huge edit list trims a the very start.

Fixe #394.

Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
arch1t3cht added a commit to arch1t3cht/Aegisub that referenced this issue Jul 1, 2022
arch1t3cht added a commit to arch1t3cht/Aegisub that referenced this issue Jul 2, 2022
CoffeeFlux pushed a commit to TypesettingTools/Aegisub that referenced this issue Jul 3, 2022
tgoyne pushed a commit to tgoyne/aegisub that referenced this issue Aug 20, 2022
dwbuiten added a commit to dwbuiten/ffms2 that referenced this issue Nov 21, 2022
…erlaced

Otherwise seeking can be jank in certain types of streams like open GOP HEVC
or MP4s with huge edit list trims a the very start.

Fixe FFMS#394.

Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
(cherry picked from commit ff61bca)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants