search

CrendKing / avisynth_filter

DirectShow filters that put AviSynth and VapourSynth into video playing
MIT License
107 stars 8 forks source link

Vapoursynth frame durations #45

Closed chainikdn closed 3 years ago

chainikdn commented 3 years ago

Please take a look at mpv's Vapoursynth filter implementation: https://github.com/mpv-player/mpv/blob/master/video/filter/vf_vapoursynth.c, specifically at the _DurationNum and _DurationDen frame properties. Actually you must use them for setting and getting back frame durations instead of our magic algorithm https://github.com/CrendKing/avisynth_filter/issues/27.

Also a few environment properties could be expected by the scripts - i.e. "container_fps" and others.

CrendKing commented 3 years ago

Since you are here, before we dive into this issue, I wonder is it possible for SVP to implement a VapourSynth variant for DirectShow players? I can verify the core functionality works if I use a hand write script, but of course all remote control does not work, as the injected code is for AVS.

If you guys are willing to do so, could you open an issue for required APIs to make it work, which are currently missing (e.g. "Is the frameserver AVS or VPS")?

If you think there is not much incentive to implement that variant, it's fine. I can certainly see the parity here.

Back to this issue. I'll take a look. When I was reading that code I didn't pay attention to that specific detail.

For other environment props, I wasn't planning to have full parity with mpv. But if you want to do the variant and those props are needed, I could consider implementing.

chainikdn commented 3 years ago

Regarding mpv's environment properties, I believe container_fps is the useful one. Regarding frame properties - you might want take ffms2 as a reference: https://github.com/FFMS/ffms2/blob/45673149e9a2f5586855ad472e3059084eaa36b1/src/vapoursynth/vapoursource.cpp#L117 and all those properties are actually from the Vapoursynth docs: https://github.com/vapoursynth/vapoursynth/blob/db25018fafef612185f79f0f86164430b0bbf6f6/doc/apireference.rst#reserved-frame-properties

I wonder is it possible for SVP to implement a VapourSynth variant for DirectShow players?

Sure it's possible, I just don't know why... I can't see any benefits over AVS+ path right now.

CrendKing commented 3 years ago

Regarding the source FPS, there is the source node's fps property. According to the doc, it should be just the container_fps if the video has fixed framerate, but "0/1" if variable. However, mpv seems to never set it so it's always 0/1? Since there is no way to know if the video is VFR by just looking at the container, so I guess that's the best practice (with compliance with VapourSynth spec).

I just feel that it's waste to introduce non-standard variables when there is already proper place to hold them. If you check the current VpsFilterSource.fps it is basically container_fps.

So maybe I should just document it that all videos are considered FFR, VpsFilterSource.fps is equivalent to container_fps, and be done with it?

CrendKing commented 3 years ago

https://github.com/CrendKing/avisynth_filter/actions/runs/840917165

chainikdn commented 3 years ago

mmm... the whole point of this issue was about incorrect frame times and they are still incorrect, because Vapoursynth filters explicitly set frame duration via those _DurationNum/Den properties, and you are not using them

CrendKing commented 3 years ago

From what I see, mpv uses ffmpeg's pkt_duration as the frame duration. And LAV Filters uses pkt_duration to set frame stop time. I currently set the two duration properties with the stop - start time, which should be exactly what mpv does, no?

Otherwise, are you suggesting we should use the stop time in lieu of our magic algorithm?

chainikdn commented 3 years ago

Output. I'm talking about output durations.

CrendKing commented 3 years ago

Sorry for misunderstanding your original request. Does https://github.com/CrendKing/avisynth_filter/commit/8d976be66528143bf6b210ac5e08fa37a7b0a544 fix the issue?

CrendKing commented 3 years ago

@chainikdn Can you verify this is really fixed? I'd like to release a version if all your issues are fixed.

chainikdn commented 3 years ago

I just looked through the code, I'm not actually using Vapoursynth filter now. If you say it's fixed - then it's fixed ;)

CrendKing commented 3 years ago

Oh, I had no doubt you don't use the VapourSynth DS filter in your code :). The way you described the issue was very decisive, which made me a bit confused at first. I'm glad we solved this eventually.

Before you go, the concern I have is that what if some VapourSynth filter does not respect the _DurationXXX properties? What if they drop these properties during their process, and never add it back? What if a fps manipulation filter (such as SVP) forget to alter these, resulting a incorrectly speeded video? I love how simple the VapourSynth frame handler logic now is, but without the magic algorithm I feel a bit naked and less assured.

chainikdn commented 3 years ago

What if a fps manipulation filter (such as SVP) forget to alter these, resulting a incorrectly speeded video?

then the filter's author will look at the incorrectly speeded video and will think what he does wrong

CrendKing commented 3 years ago

Fair enough. However, the reference page states "It is acceptable to not set any of these keys if they are unknown". It doesn't say it is not acceptable to not set these if they are known. I hope the VapourSynth guys were more strong-handed on these quasi standards.

I also found that AviSynth wiki says these properties are backported to AviSynth side: http://avisynth.nl/index.php/Internal_functions#Reserved_frame_property_names, available since Avs+ 3.6. Unfortunately Avs+ 3.5 doesn't work with them. And obviously SVP doesn't support these.

Any chance you guys can upgrade to Avs+ 3.6 or above in the future? Could make many people happy :)

chainikdn commented 3 years ago

if the keys are not set (which is unlikely) then you could take 1/fps as a frame duration as fallback

Any chance you guys can upgrade to Avs+ 3.6 or above in the future?

avs 3.6 conflicts with ffdshow, so... only when ffdshow will be completely removed

CrendKing commented 3 years ago

@chainikdn I have a question. Related to issue https://github.com/CrendKing/avisynth_filter/issues/56.

From VapourSynth's RFP page, there is also the _AbsoluteTime property, which indicates the frame start time. The description specifically says "Should only be set by the source filter and not be modified".

So imagine SVP is doubling the FPS of a video. VPFS generates frames 1, 2, 3 ... with their respective _AbsoluteTime. SVP gets these frames and generates output frames 1, 2, 3, 4, 5, 6 ... I notice output frame 1 and 2 will have exactly the same _AbsoluteTime as the source frame 1. 3, 4 same for source 2.

Problem is, SVP is the source of these interpolated frames (frame 2, 4, 6). Shouldn't SVP set their _AbsoluteTime accordingly? Otherwise, when output frames 2, 4, 6 come back to VPFS, how do I know their proper start time? I hate to revert to counting myself like AVFS because all other aspects are now using RFP.

chainikdn commented 3 years ago

I think this more a theoretical question cause this "_AbsoluteTime" is not using anywhere. In the Vapoursynth's repo search for "_AbsoluteTime" and you'll see only one match - in the readme :D mpv doesn't use it, built-in filters like AssumeFps don't adjust it.

Ahhh, missed that one - https://github.com/CrendKing/avisynth_filter/blob/b686043bd1a3711d6261b433beeb051bd4d0716c/vapoursynth_filter/src/frame_handler.cpp#L300 I think this is really wrong.

CrendKing commented 3 years ago

That's why I'm fixing with https://github.com/CrendKing/avisynth_filter/commit/5e0ecff98e251f65fa3c462f5021e1f7802c1c35

Well, FFMS uses it: https://github.com/FFMS/ffms2/blob/55c2af57f1bdc587ca98d2e28d3d764c00b3e13a/src/vapoursynth/vapoursource.cpp#L140, which is a real source filter. So the consensus is that only the very root filter should put this property but not the intermediate filters?

If that's the case, guess my fix is needed, even though I think the "consensus" is wrong. Asking in VPS: https://github.com/vapoursynth/vapoursynth/issues/702

chainikdn commented 3 years ago

I think this property is just an optional info field, "just in case" some filter would need the absolute time. Nothing more.

-- btw AssumeFps doesn't interpolate frames, it only rewrites duration num/den

CrendKing commented 3 years ago

I agree that RFP are optional to begin with, so I'm not saying SVP is required to add it. I'm more of requesting SVP to add it if _AbsoluteTime is present in the source frame, because the inter frames are generated by SVP, it has the most pristine knowledge of the metadata. VPFS can calculate, but it is calculated based on the assumption that "the start time of the next frame must equal to the stop time of the previous frame".

OK. I was thinking of AviSynth's ChangeFPS. Well that explains why AssumeFps doesn't add _AbsoluteTime.

chainikdn commented 3 years ago

my point is _AbsoluteTime is useless after AssumeFps, even if it was set it only reflects source (i.e. "container") timestamp

CrendKing commented 3 years ago

Hmm, forgot about the frame drift problem. You are right, VPFS needs to aggregate.