MeGUI development

[Richard Berg]

I wouldn't take any of these rules too far. Exceptions and return values are just tools; good programmers simply choose the best one for the job. Worrying that you'll trigger an exception often enough that someone might call it "normal" is premature optimization, which we know is the root of all evil.

In this case, forcing the immediate caller of IJobProcessor.Setup to always test the return value works ok with our design, so no exception necessary. If IJobProcessor.Setup had dozens of possible entry points, using an exception would be better because it guarantees that someone in the call stack will do something about it.

[dimzon]

2 Doom9, Richard Berg, Mutant_Fruit

and you shouldn't use exceptions for normal program flow..

<offtopic>
Does and you know how does woking FileMapping/VirtualMemory/PageFile mechanism? Whe you trying to addreess to non-existing page in RAM processor will generate hardware interrupt (exception). Windows catch this interrupt, look for this page in PageFile, load it to the memory. So exceptions (sometimes) is a really fine to be used in normal program flow...
Another sample - for ASP.NET programmers. When You invoke Response.End exception is trown, really, try this code

try
{
Response.End();
}
catch(Exception e)
{
Response.Write(e.ToString());
}

And yet another sample for threadind: When you invoke thread.Abort() ThreadAbordet exeption is throwing
</offtopic>

But my sample IS NOT A NORMAL PROGRAM FLOW.
If JobProcessor can't setup it mean:

• You trying to setup JobProcessor for unsupported job
• Job configuration is invalid
• JobProcessor configuration is invalid
• Another shit happens

All options is exceptions, isn't it...

With IJobProcessor.Setup, I think using an error parameter is appropriate. Any method that calls Setup should know what to do if it receives an error string.

I really don't think You will have multiple places to call IJobProcessor.Setup, but if You really want to do it end you does'nt want to wrap your code using try/catch @ every place you can use PATTERN-ADAPTER for it:

public interface IJobProcessor
{
   void Setup(Job job);
}

public class JobProcessorAdapter
{
   private IJobProcessor _proc;
   public JobProcessorAdapter(IJobProcessor  p)
   {
      _proc = p;
   }

   public bool setup(Job job, out string error)
   {
      try
      {
        _proc.Setup(job);
        return true;
      }
      catch(Exception e)
      {
        error = e.Message;
        return false;
      }

   }  
}

2 All developers

I have found another ugly code:

MyForm f = new MyForm();
if(f.ShowDialog()==DialogResult.OK)
{
// Do something
}

Proper way:

using(MyForm f = new MyForm())
{
  if(f.ShowDialog(this)==DialogResult.OK)
  {
  // Do something
  }
}

[Doom9]

I have found another ugly code:

What is the problem with it? The local method variable will go out of scope as soon as you exit the method and will be garbage collected..

[dimzon]

Originally Posted by Doom9

What is the problem with it? The local method variable will go out of scope as soon as you exit the method and will be garbage collected..

Form implemets IDisposabe interface. It means it hold unmanaged resources. Unmanaged resources can't be collected via garbage collector.
Really, garbage collecter will be called only @ deficit memory. But you object can hold window handler or other handles, SqlConnections e.t.c.

Read this:
http://msdn.microsoft.com/library/de...ASP?frame=true
http://msdn.microsoft.com/library/de...ASP?frame=true
http://msdn.microsoft.com/library/de...asp?frame=true

http://msdn.microsoft.com/library/de...classtopic.asp
http://msdn.microsoft.com/library/de...pc10192000.asp

[dimzon]

Dear Doom9.
MeGUI invokes DGDecode.dll direcly. It causes such problem:
DgDecode.dll must be im MeGUI folder or be avaluable @ PATH enviroment string.
In other hand DGDecode.dll MUST be @ avisynth\plugins folder in order to be able open scripts.
So I want to propose solution - let's open d2v files via AviSynth temporaly script file.

[Mutant_Fruit]

Originally Posted by dimzon

Whe you trying to addreess to non-existing page in RAM processor will generate hardware interrupt (exception)

Aye. This is what i would classify as an "exception". You've tried doing something that is 'impossible'. You can't access a non-existing page, so rightfully an exception should be thrown. This would be normal program flow.

But in this example assume the string "str_value" usually contains an integer, but could occasionally be null....

try 
{
	int result = Convert.ToInt32(str_value);
}
catch
{
	int result = -1; //the string was null or contained invalid data
}

Thats the wrong way of doing it. The right way is this:

try 
{
	if(str_value != null)
		int result = Convert.ToInt32(str_value);
}
catch
{
	int result = -1; //string contained invalid data
}

You check to see if it's null BEFORE trying to convert it. That way exceptions will only happen when an "unexpected" event happens. Being null is an expected event that should be checked for.

[dimzon]

@Mutant_Fruit

But in this example assume the string "str_value" usually contains an integer, but could occasionally be null..

agreed 100%. But IF You cange this stament to

But in this example assume the string "str_value" MUST contains an integer

then

try 
{
	int result = Convert.ToInt32(str_value);
}
catch
{
	int result = -1; //the string was null or contained invalid data
}

is valid.

    * You trying to setup JobProcessor for unsupported job
    * Job configuration is invalid
    * JobProcessor configuration is invalid
    * Another shit happens

You MUST call JobProcessor for supported job type only
You MUST provide valid job
You MUST provide valid configuaration

So using exceptions in that case is good, is'nt it?

[Doom9]

* You trying to setup JobProcessor for unsupported job

Can't happen

* Job configuration is invalid

Can't happen

* JobProcessor configuration is invalid

Can't happen

* Another shit happens

Like?

The only thing that can go wrong in the setup is that the encoder cannot be found.. other than that, nothing can go wrong. It is the job of a jobdispatcher to send jobs to where they belong (yeah I know you hate the "is" type checks.. but they ensure this never ever happens). Similarly, it must not be possible to create invalid jobs.. it's the job of the jobcreator to make sure it returns a valid job, or nothing at all.. incidentally it's done that way.. jobs that would be invalid are not returned, instead you get a null value and it's not added to the queue.

[dimzon]

Originally Posted by Doom9

Can't happen

No, it can. Executable path is JobProcessor configuration.

Originally Posted by Doom9

Like?

Don't know. Some shit (maybe) @ some implementation...

[Mutant_Fruit]

Originally Posted by dimzon

@Mutant_Fruit

agreed 100%. But IF You cange this stament to

"But in this example assume the string "str_value" MUST contains an integer"

then

try 
{
	int result = Convert.ToInt32(str_value);
}
catch
{
	int result = -1; //the string was null or contained invalid data
}

is valid.

Aye. In that case it would be valid. But in my example we knew that at times the input could be null and at times it could contain an int. In that case we should check if it is null BEFORE we try to parse out an int to avoid throwing an exception when the str_value is null. In my example, a null value can be expected and so shouldn't throw an exception.

In your example, you are relying on an int being contained in the string (str_value MUST contain an int). Since the int MUST be there, an exception should be thrown if an int is not there (i.e. the string is null or contains text).

In my example, if the string was null i could return a value indicating a fail result, but i wouldn't use an exception to let the program know that the string was null.

bool ParseOutMyInt(string str_value)
try
{
	if(str_Value != null)
		result = Convert.ToInt32(str_value);
	else
		return false; // str_value is null, therefore not parsable

	//do rest of code and return true for success
}
catch
{
	return false; // it failed somewhere in the code.
}

[Richard Berg]

Guys, at the level you're worrying about, it's just a style preference.

[Doom9]

Form implemets IDisposabe interface. It means it hold unmanaged resources. Unmanaged resources can't be collected via garbage collector.

I think you're misreading the interface definition. Implementing IDisposable means you give the GC a method to call to make sure everything is being cleaned up. There's no need to call finalize for any of these classes listed if you are properly cleaning up after yourself. E.g. if you close the StreamWriter there's no need for using.. it just makes the close implicit. As long as your form doesn't hold a handle, or keeping streams/connections open, there's no problem with the garbage collection (at least I didn't find anything in the articles you linked to).

Where native resources are used, they should be properly cleaned up (but obviously nobody's perfect and I might have missed something.. but for instance the avi reader implements additional cleanup stuff because it goes native).

No, it can. Executable path is JobProcessor configuration.

That's not the JobProcessor configuration.. it's the MeGUI configuration. I think it's pointless to argue about these interfaces.. they may not be final, but it's what it is now.. I'd really like you to commit some code instead of always trying to run headfirst against the wall that is me opposing changes all over the place. The whole job launching mechanism is bound to be changed in the future anyway.. and I don't like changes over and over. I only changed the encoding stuff because I needed to accomodate more encoders.. and bringing everything to the same level reacquainted myself with the code that I might have to modify in the future.

Since the risk is rather high that I'll do considerable parts of the refactoring in the future as well.. it really shouldn't be a cause to step on the brakes (which is what I feel you're doing.. everybody else here has commited some code for the current "broken" architecture). A major architectural overhaul is bound to take weeks if not months and I'm not the one to put development on hold just because things aren't very pretty right now.. they still work after all.

So I want to propose solution - let's open d2v files via AviSynth temporaly script file.

No.. I love my dgdecode code...not only was it a major achievement for me (I've never written interop code without any help before), and wrapping it into AviSynth just adds another dependency. Your solution is what I consider the cheap way.. other softwares do that.. I prefer to do it the hard way. And why bother at all.. it works, it has never caused any problems. It seems just another instance where you want to run headfirst in a wall instead of letting things that work be. I've made the decision to wrap dgdecode a long time ago fully aware (and with already existing code to get frames via avifile) and I'm not about to reverse that now unless you come up with an awsome reason that blows me away.. the path thing is in my "I couldn't care less" category.

[Doom9]

Guys, at the level you're worrying about, it's just a style preference.

That's what I'm saying.. let's keep thing that work the way they are. In the end, it's still my baby in a way so as long as I keep my fingers in MeGUI, you'll always have certain things that reflect how I approach a problem.. I have never used custom exceptions (well.. except when going through the mcad courses), so I'm unlikely to start now because I just don't like try/catch code and like to handle exceptions at low levels.

It's similar for the interfaces.. if I'm going to change them.. I'll have to change all classes implementing them because a CVS commit must not break compilation.. so it's my problem in the end and I can live with that.

[Mutant_Fruit]

Just put the new version of the ContextHelp.xml file (written by LiFe) up on SF. Also made a few changes to the contexthelp code to account for the fact that linebreaks are picked up from the XML correctly.

And i moved some checkboxes slightly as there was a bit of overlap.

[Doom9]

by the way, how would you like that I feed the cutlist to the avisynth based audio encoder? In the video world it's probably an array with two ints per line, a start and end frame. I figure just before encoding I open the AviSynth script, and put a trim line at the end of the script.

[stax76]

I think you're misreading the interface definition. Implementing IDisposable means you give the GC a method to call to make sure everything is being cleaned up. There's no need to call finalize for any of these classes listed if you are properly cleaning up after yourself.

I'm pretty clueless about the topic but do you confuse Dispose with Finalize? I think the GC calls Finalize but not Dispose. Using ensures Dispose is called under every circumstance, even when a exception happens similar like (try/catch) finally which is also executed in every circumstance (exception, break, goto...).

[dimzon]

Originally Posted by Doom9

there's no problem with the garbage collection (at least I didn't find anything in the articles you linked to).

FYI: Windows form holds window handles & GDI handles even if it was unvisible!
Really IDisposable.Dispose performs cleanup(line finalizer) + remove futher finalizer invokation via GC.SuppressFinalize().

Finalization and Performance

With this basic understanding of finalization we can already deduce some very important things:

First, objects that need finalization live longer than objects that do not. In fact, they can live a lot longer. For instance, suppose an object that is in gen2 needs to be finalized. Finalization will be scheduled but the object is still in gen2, so it will not be re-collected until the next gen2 collection happens. That could be a very long time indeed, and, in fact, if things are going well it will be a long time, because gen2 collections are costly and thus we want them to happen very infrequently. Older objects needing finalization might have to wait for dozens if not hundreds of gen0 collections before their space is reclaimed.

Second, objects that need finalization cause collateral damage. Since the internal object pointers must remain valid, not only will the objects directly needing finalization linger in memory but everything the object refers to, directly and indirectly, will also remain in memory. If a huge tree of objects was anchored by a single object that required finalization, then the entire tree would linger, potentially for a long time as we just discussed. It is therefore important to use finalizers sparingly and place them on objects that have as few internal object pointers as possible. In the tree example I just gave, you can easily avoid the problem by moving the resources in need of finalization to a separate object and keeping a reference to that object in the root of the tree. With that modest change only the one object (hopefully a nice small object) would linger and the finalization cost is minimized.

Finally, objects needing finalization create work for the finalizer thread. If your finalization process is a complex one, the one and only finalizer thread will be spending a lot of time performing those steps, which can cause a backlog of work and therefore cause more objects to linger waiting for finalization. Therefore, it is vitally important that finalizers do as little work as possible. Remember also that although all object pointers remain valid during finalization, it might be the case that those pointers lead to objects that have already been finalized and might therefore be less than useful. It is generally safest to avoid following object pointers in finalization code even though the pointers are valid. A safe, short finalization code path is the best.

FYI: Invoking IDisposable.Dispose() is good programming style anycase.

Originally Posted by Doom9

That's not the JobProcessor configuration... it's the MeGUI configuration.

Yes, Okay, I can call it

JobProcessor related MeGUI configuration is invalid

instead of

JobProcessor configuration is invalid

Originally Posted by Doom9

always trying to run headfirst against the wall that is me opposing changes all over the place

I'm afraid You get me wrong (my poor english can be a reason). I'm not forcing You to make any changes (and I'm not trying to do them myself) - it's just a discussion

Originally Posted by Doom9

No.. I love my dgdecode code...not only was it a major achievement for me (I've never written interop code without any help before)

I understand your motivation (but I'm complete disagree with You).

Originally Posted by Doom9

and wrapping it into AviSynth just adds another dependency. Your solution is what I consider the cheap way...

Cheap != Wrong. And don't forget - You are using DGIndex.dll for AviSynth script generation - isn't it? Using DGIndex.dll is useless without AviSynth, so opening it via AviSynth is valid way.

Originally Posted by Doom9

the path thing is in my "I couldn't care less" category.

Just take look @ SettingsForm.cs. There are ugly code to work with PATH.
And how about DllHell (one version @ AviSynth plugin folder, another @ DGIndex folder, third @ MeGUI folder). Using AVS-way you are ensure - AviSynth plugin folder version are used!

[dimzon]

Originally Posted by Doom9

by the way, how would you like that I feed the cutlist to the avisynth based audio encoder? In the video world it's probably an array with two ints per line, a start and end frame. I figure just before encoding I open the AviSynth script, and put a trim line at the end of the script.

Just a question - What does start and end frame means for audio?

[Doom9]

Using ensures Dispose is called under every circumstance, even when a exception happens similar like (try/catch) finally which is also executed in every circumstance (exception, break, goto...).

my code that makes access to resources where this could be a problem is structured such that any failures are handled. I came from C# from Java where there's no using, so I'm using a pattern that works on both platforms and cleans up after itself in all cases... exceptions included. So instead of arguing semantics.. either prove with current code where I'm not cleaning up properly, or drop it.. I'm about to lose my motivation to consider making any changes at all. you're back to the let's do everything different ways I told you in the beginning I was afraid of. Why don't you do something that actualy benefits the user like adding the avs based audio encoder and answering my question on how you want the cutlist..
It is a bad idea to take an existing project and change everything to your coding style.. what if the person whose style it corresponds leaves in midstream? What if you don't see it through? Don't answer that.. I've run a site for almost 6 years now and there's been only one other person I've been able to completely rely on.. all others have come and gone.. so that's what I expect will happen with MeGUI.. it's the open source approach.. people come and go.. but I know I'll stick around. Making changes just because they fit somebody's coding style better are useless changes for a project.

[dimzon]

Originally Posted by Doom9

either prove with current code where I'm not cleaning up properly, or drop it...

		private void mnuToolsSettings_Click(object sender, System.EventArgs e)
		{
#if FULL
			SettingsForm sform = new SettingsForm(this.videoProfiles, this.audioProfiles, 
				this.videoProfile.SelectedIndex, this.audioProfile.SelectedIndex);
#else
			SettingsForm sform = new SettingsForm();
#endif
			sform.Settings = this.settings;
			if (sform.ShowDialog() == DialogResult.OK)
			{
				this.settings = sform.Settings;
                this.shutdownCheckBox.Checked = this.settings.Shutdown;
                this.saveSettings();
                // this is here to prevent output extension mismatches when the x264 encoder is changed
				videoOutput.Text = changeVideoOutputExtention(videoOutput.Text); 
			}
		}

sform still hold unmanaged GUI resources (windows handles, etc) until it be garbage collected. But it will no garbage collector invocation until deficit of memory. Deficit of memory != deficit of windows handles (.NET Runtime can't say - "ok, there are not enought GDI resources, let's collect garbage").