search

CrendKing / avisynth_filter

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

Add more fields to the status page #2

Closed chainikdn closed 3 years ago

chainikdn commented 3 years ago

Do you really have to argue on every single line, even though it is inline with your code-style and doesn't affect core logic?

Talking about the "core logic":

Wouldn't inSampleFrameNb the more accurate stat? That's directly from the source filter.

The whole numbers/timestamps logic seems wrong. Specifically inSampleFrameNb is wrong not only in case of VFR but for CFR too, because you divide timestamp by the output frame rate value. Which is obviously different from input value...

chainikdn commented 3 years ago

Whenever possible, it is better to just return and assign than changing the variable inside.

I'll later need to pass all these values to the "remote control". When the time comes, it could be refactored somehow.

There could be more fields in the future.

There won't be any more fields ever :D

CrendKing commented 3 years ago

Do you really have to argue on every single line, even though it is inline with your code-style and doesn't affect core logic?

This is how my company trained us to review code. I'm just sharing what I'd do if I'm writing the code. Never thought it would offend someone. Please feel free to ignore.

Talking about the "core logic":

Wouldn't inSampleFrameNb the more accurate stat? That's directly from the source filter.

The whole numbers/timestamps logic seems wrong. Specifically inSampleFrameNb is wrong not only in case of VFR but for CFR too, because you divide timestamp by the output frame rate value. Which is obviously different from input value...

I'm using the output frame rate because that's the frame number I'm sending to the avs clip. Say the script converts fps from 30 to 60, and I'm getting the 20th input frame. Should I request frame #20 or #40? Because if #20, that's actually the intepolated frame between #10 and #11.

There won't be any more fields ever :D

I'm sorry. Didn't mean to offend you. I was referring to ffdshow, which has aspect ratio, bitrate and other stats.

chainikdn commented 3 years ago

Never tried to talk about offense, defense or any of these... I'll do my best to follow your style and you feel free to silently fix any of my commits, ok ? This will save time for both of us :)

===

  1. VIDEOINFOHEADER::AvgTimePerFrame is an average time per frame. This's only an informational value, and it may be completely wrong.

  2. Consider CFR 30 fps source (AvgTimePerFrame ~= 30 ms) and x2 AVS script -> 15 ms per frame. inSampleFrameNb = input timestamp / 15 ms, while the actual input frame number must be 2 times smaller

  3. 24-30 VFR source (AvgTimePerFrame set to 30 ms, but the real frame times are between 30 and 40 ms), 1:1 AVS script input frame 0 starts at 0, frame 1 at 30, frame 2 - 75, frame 3 - 115, frame 4 - 150 you'll output frames at: 0, 30, 60, 90, 120 - which is wrong

  4. same as 2, AVS requested frame number 3. In SourceClip::GetFrame() you convert frame number 3 into timestamp 3*30 = 90, and will actually return frame 2 (as its timestamp of 75 is closer to 90)

CrendKing commented 3 years ago

you feel free to silently fix any of my commits, ok ? This will save time for both of us :)

I didn't know if people are OK with this. The colleagues in my company don't like people to touch their precious code so I have to list every minor detail in review. I don't do many PR in GitHub before. Is this the culture outside?

As for the CFR, I get it now. I'll merge this first then do that update.