search

CrendKing / avisynth_filter

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

Remote control, HDR and other stuff #4

Closed chainikdn closed 4 years ago

chainikdn commented 4 years ago

The frame rate is still inaccurate especially for VFR.

chainikdn commented 4 years ago

If the exact same avs file is loaded again, the input format would be exactly same. Why force a format change handling?

This is the shortest way of reloading everything when the script is changed. No idea why someone will want to load "the exact same avs file" again, but anyway it wouldn't harm to "change" the format too in this case.

Why is reloading the avs file without reloading the env safe, if user changed the script content?

I believe the environment will be reloaded too, from HandleInputFormatChange().

chainikdn commented 4 years ago

And again, the RC part is made so it act exactly like a PotPlayer. This includes message ids, not a "message-only window" and Eval() instead of Import().

chainikdn commented 4 years ago

Since the Eval() branch can only be used when RC is enabled

Not sure... right now you can place an Avisynth code into the "path" field and it probably should work. Looks like a feature. not a bug :D

CrendKing commented 4 years ago

Suggestion: Please directly respond in each conversation so I know which one it is about.

This is the shortest way of reloading everything when the script is changed. No idea why someone will want to load "the exact same avs file" again, but anyway it wouldn't harm to "change" the format too in this case.

I think I should have commented on that force argument of HandleInputFormatChange(). I agree the change in Receive() is necessary. But force is not needed. If the new format is different, even without force it will refresh state. If the new format is the same, force only refreshes state unnecessarily.

And again, the RC part is made so it act exactly like a PotPlayer. This includes message ids, not a "message-only window" and Eval() instead of Import().

I understand it is currently exactly like PotPlayer. But I don't like some part of the way it implements it (e.g. RC taking over avs file, using both WM_USER and WM_COPYDATA, etc). Are you planning to change the code or this is it? Of course we can discuss the part that I don't like and see if we can reach some agreement.

Not sure... right now you can place an Avisynth code into the "path" field and it probably should work. Looks like a feature. not a bug :D

You mean after this PR right? If we plan to support both an avs file and avs script, we should properly have two controls (one single-line edit that accepts file drop, one multiline edit) in GUI to convey the design instead of vaguely allowing script in a path inputbox. When I originally created the filter, I had debate between supporting script text exactly as ffdshow and PotPlayer and only script file. My reason for choosing script file was that 1) editing in an EDIT is terrible user experience comparing to proper AVS editor or even generic text editor; 2) application such as SVP already generates avs file to be loaded; 3) most users may not write scripts themselves anyway but just download other people's code; 4) if user comes up with complicated script, storing large amount of data in registry is not idiomatic.

I'm OK if RC adds additional script text loading behind the door, so long as the general GUI design stay that way.

chainikdn commented 4 years ago

If the new format is the same, force only refreshes state unnecessarily.

Remember, the script was changed, and the output was (likely) changed. And all those "replaceOutputType" stuff are necessary. The whole point is to add as little code as possible. We're just pretending the input format was changed, which leads to script reloading and everything.

But I don't like some part of the way it implements it (e.g. RC taking over avs file, using both WM_USER and WM_COPYDATA, etc

Dunno what the problem is... COPYDATA is for sending a data block, ordinary messages are for simple one-number commands. ffdshow works the same way. Anyway, we can discuss this later... I really don't want to merge and "rebase" this one more time ;)

chainikdn commented 4 years ago

Do you really want me spent another hour merging this?

=== In fact, it doesn't work as before after merging.

CrendKing commented 4 years ago

No, don't worry about merging. I'll do it myself later. In fact, I felt bad about asking you to do the first merging. It should have been my responsibility. I think contributor should do merging only if there are some exquisite pieces of code in the PR that are difficult to understand and tightly integrated to the existing system. This PR is largely new feature, so I should be able to do.

CrendKing commented 4 years ago

Just FYI, I merged your commits before https://github.com/CrendKing/avisynth_filter/pull/4/commits/a82fdde845bd91cb8e77414018a3c94708f2cd26 locally. Tested a bit and working.

I see you made 4 more commits. May I ask do you plan for more commits? If yes, I'll wait for you to finish. Otherwise I'll tend to them. Thanks.

chainikdn commented 4 years ago

I think I'm good for now.

chainikdn commented 4 years ago

Sorry, can't stop 🤣 If I were you I'd merged this two weeks ago and then fixed whatever I didn't like.