search

zachsaw / MPDN_Extensions

Media Player .Net (MPDN) Open Source Extensions
Other
58 stars 18 forks source link

Framework Changes + DebandExperimental update + Audio Crossfeed filter #238

Closed Shiandow closed 7 years ago

Shiandow commented 7 years ago

Framework Changes

New method for chroma scaling (doesn't require too many changes to existing chroma scalers). Allows filters to treat Luma and Chroma channels separately if so desired (see DebandExperimental).

Tag system: changed, again. This time to a more foolproof system.

Filter: Disposing accidentally took exponential time, fixed to linear time.

TextureSize: Add comparison methods, remove IsUpscalingFrom, IsDownscalignFrom in favour of more explicit inequality operators. Operators return true when inequality is true for all dimensions.

Audio/Video Scripts

DebandExperimental: tweaks, allow debanding to be done before chroma scaling, improving quality.

SSIMdownscaling: minor tweak

Crossfeed: new audio crossfeed. More of an experiment, and to demonstrate the relative ease with which you can process audio (although actually producing good sounding audio is obviously still black magic).

Shiandow commented 7 years ago

I'll give this a final check some time tomorrow.

zachsaw commented 7 years ago

Whoa! Massive improvements!

I'll give it a quick test tonight!

On 29 Aug. 2017 8:33 am, "Shiandow" notifications@github.com wrote:

Framework Changes

New method for chroma scaling (doesn't require too many changes to existing chroma scalers). Allows filters to treat Luma and Chroma channels separately if so desired (see DebandExperimental).

Tag system: changed, again. This time to a more foolproof system.

Filter: Disposing accidentally took exponential time, fixed to linear time.

TextureSize: Add comparison methods, remove IsUpscalingFrom, IsDownscalignFrom in favour of more explicit inequality operators. Operators return true when inequality is true for all dimensions. Audio/Video Scripts

DebandExperimental: tweaks, allow debanding to be done before chroma scaling, improving quality.

SSIMdownscaling: minor tweak

Crossfeed: new audio crossfeed. More of an experiment, and to demonstrate the relative ease with which you can process audio (although actually producing good sounding audio is obviously still black magic).

You can view, comment on, or merge this pull request online at:

https://github.com/zachsaw/MPDN_Extensions/pull/238 Commit Summary

  • [Framework] Split AudioFilter into separate classes
  • [Framework] Add Size and Format helper functions
  • [Refactoring] Cleanup
  • Ignore VS 2017 temp files
  • [Framework] Complete rewrite of Tag system
  • [Framework] SetSize: check if resize is necessary
  • [Framework] ExtensionUI: add SettingsChanged event
  • [Framework] Filter: Speed up Disposing
  • [Framework] Add comparison methods for sizes
  • [Framework] Chroma Scaling: Rework chroma filter, allow acces to YUV and Luma planes.
  • DebandExperimental: Handle Luma and Chroma separately.
  • SSIMDownscaler: Small tweak, update scalers
  • Crossfeed: Work in Progress

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/zachsaw/MPDN_Extensions/pull/238, or mute the thread https://github.com/notifications/unsubscribe-auth/ACqEcLq-G1KOSrLrry0ZltV_KAsejyH9ks5sc0A_gaJpZM4PFJzI .

Shiandow commented 7 years ago

By the way, it might not seem like much, but by far the most noticeable feature is the fix that made disposing filters faster. It makes a very noticeable difference when you toggle renderscripts on/off using hotkeys.

Shiandow commented 7 years ago

For whatever reason I'm getting problems when I try to let MPDN compile the extensions (using "Validate Release.bat"), but manual compilation seems to work fine (even when targetting C# 5.0), weird.

Shiandow commented 7 years ago

I actually wondered about that. What class would be need to prevent C# from actually storing anything? If you return an enumerator it just generates the results on the fly, but I couldn't figure out what class to extend to make that happen.

zachsaw commented 7 years ago

IEnumerable would be the bare minimum. GetEnumerator would simply just yield return each comparison result.

zachsaw commented 7 years ago

In fact if you implement it as a simple IEnumerable, you can then add an extension method All(bool)/Any(bool) that would work with anything that implements the same interface.

zachsaw commented 7 years ago

IEnumerable

zachsaw commented 7 years ago

Buggery GitHub doesn't like IEnumerable < bool > without the spaces!

Shiandow commented 7 years ago

Not sure if I like "All(true)". Or rather I don't like "All(false)", and if we don't allow that then the "true" is redundant.

There's also the fact that All(x=>x) is the same length...

And yeah you need to write IEnumerable\\<bool\> for some reason.

zachsaw commented 7 years ago

Let me quickly do it and push the changes

zachsaw commented 7 years ago

I think the following syntax is pretty easy to understand,

if ((a<b).All(false)) ...

Any particular reason why you don't like it?

I don't mind lengthy code, as long as the intent is clear.

Shiandow commented 7 years ago

At this point I'm starting to doubt if the extra code is actually clarifying anything.

I mean yes it is explicit, but if you add too many ways of saying the same thing you could end up with code like "!(a < b).Any(false) == true". Now obviously this could be simplified, but I'm just using it to demonstrate that being explicit and being easy to read aren't the same thing. Unless you can claim you immediately saw that that code was equivalent to "a < b" in the old syntax.

zachsaw commented 7 years ago

Just to be clear, I'm not in favour of being explicit - I only want the code to express intent clearly. My only problem with (a<b).Any() is that it hides the .Net implementation of IEnumerable.Any() - and with that, anyone is familiar the extension method .Any() would be confused wondering which version of Any() we're referring to when reading our code.

zachsaw commented 7 years ago

Hmm I can see why you implemented Any and All as properties - that would actually be quite enough to differentiate between the extension method and one that is for ListOfBools.

Shiandow commented 7 years ago

It is a bit of a compromise, but it's the best I could come up with.

There's also no risk of accidentally calling the wrong Any() on an IEnumerable\<bool>.

Shiandow commented 7 years ago

I even added summaries, just in case someone wonders what the difference between Any and Any() is ;-).

zachsaw commented 7 years ago

There's some syntax that is newer than C# 5 that the pre-Roslyn compiler doesn't like. I can't seem to be able to find a convenient way of making the IDE tell us which is the offending syntax though...

EDIT: This is not true. I forced the IDE to allow C#5 syntax only and the validation still fails...

Shiandow commented 7 years ago

For whatever reason I don't get any errors if I manually tell VS it is C# 5.0.

I suppose adding / removing parts of the code should at least allow us to figure out where the problem is, I'll give that a try.

Shiandow commented 7 years ago

Well, it's somewhere in the framework apparently.

Shiandow commented 7 years ago

Ah, there are problems in Tag.cs.

And I just found out we had a debug dialog, that actually said what the error was. Probably not a good sign if we both forgot. Maybe we should just always enable it, no point in hiding errors.

zachsaw commented 7 years ago

Yikes! We have a debug dialog? Lol. Right. Absolutely no recollection of it.

Shiandow commented 7 years ago

Yeah...

I actually had to load the code of MPDN itself into the debugger before I discovered that bit of useful information.

Now, maybe we should just polish the dialog it a bit and make it the default? I don't see the point of ever throwing away the exception message. Even if people don't understand a word of it, it at least we won't have to bother explaining how to get useful error messages.

zachsaw commented 7 years ago

Yeah absolutely. Never hide exceptions.

I think enabling that debug dialog causes all exceptions to be shown in a non user friendly way though.

Shiandow commented 7 years ago

Yeah I think the user friendliness could take some work, but keep in mind that we're likely to get screenshots of whatever we end up choosing, so we should make sure it contains at least some useful information.

zachsaw commented 7 years ago

Perhaps it would be a good idea to show the debug dialog only when scripts fail to load?

Shiandow commented 7 years ago

Well, I don't mind if some types of exceptions are caught elsewhere and use their own, more user friendly, dialog. But anything that slips through should be displayed fully.

The uninformative error message the debug dialog adds could probably be removed though.

What we could do is keep the debug setting (but rename it), and use it to decide whether to wrap the exception in a more user friendly one, or keep it as is. And maybe limit the number of levels of additional information the exception dialog displays when "debug information" is turned off.

Shiandow commented 7 years ago

Something like this.

zachsaw commented 7 years ago

Yeah I agree with showing debug info when it's an unhandled exception while showing user friendly ones where it's caught.

From memory I think that's what I've done. That's always been my way of exception handling. It's just in this case I opted to catch the exception and show a user friendly dialog. But that's just from my very vague memory.

Shiandow commented 7 years ago

The main problem was that it was shown in a regular MessageBox, as opposed to an ExceptionMessageBox. So there's no way to access the inner exceptions.

I think displaying them like this is probably not a bad choice. The dialog should only ever be used when MPDN itself causes an exception, or the extensions throw an unhandled exception at MPDN. Both aren't really supposed to happen.