MeGUI development [Archive] - Page 32

Doom9

26th January 2006, 15:24

So I propose to remove such code:

* AviReader.cs - actually we does not need it, AvsReader works fine. Even if you need to open non avisynth file you can use "AviFile(fileName)" without creating temporaly file
* DirectShow.cs - now we can just use "DirectShowSource(fileName)"
* d2vReader.cs - now we can just use "LoadPlugin(...)\r\nMpeg2Source(fileName)"
Isn't AviReader already not in use anymore? If so it would only make sense to remove it. But keep in mind, I want that YV12 warning message before that.. it's way more important than cleaning up.. cleaning up only helps developers.. colorspace checking helps the user.
I can't comment on the DirectShow.cs file.. I don't know where it's being used and I've never even taken a look at what's inside.
As far as the d2vreader goes.. it's not done by switching a few statements.. the preview is closely connected to the reader so I have my reservations.. and come to think of it.. we get the DAR from the d2v.. you can't get that from AviSynth, can you? If you load the d2v via AviSynth, your 720x576 input becomes a 1:1 DAR.. you don't know whether it's a 4:3 or 16:9 source anymore, and that information is quite crucial, and you can't get it anywhere else reliably (keep in mind, we don't always have the info file and the info file only applies to one click mode.. not the avisynth script creator). So effectively, by eliminating the d2v reader, you introduce another manual step: DAR selection.. and since people don't have to run dgindex manually, they never get to see that.. so effectively either they know themselves, or they get the info from a tool that displays it.

Richard Berg

26th January 2006, 15:32

Being able to read d2v's shouldn't be cut. In addition to Doom9's point, we need the info in them in order to give the user helpful error messages. For instance, a very common problem is that d2v's hardcode the full path and don't work with relative paths. Simple for MeGUI to read the full path & check for file existence; hard for the user to debug without help.

dimzon

26th January 2006, 15:45

we get the DAR from the d2v.. you can't get that from AviSynth, can you?
Really d2vReader contains 2 independend blocks of code
1) PInvoke around DGDec.dll (it's really buggy, i got GPF @ trying to open d2v created by older version on DGIndex. AviSynth in other hand provide valid diagnostic message)
2) Direct d2v parsing via opening text stream

I suggest to remove PInvoke around DGDec.dll and replace it to AviSynth invocation... Or, maybe, better way is to move d2v parsing code into separate class d2vParser (bcz I found some code with direct d2v parsing, we can collect all related code in d2vParser) - and remove d2vReader at all and use AvsReader when you need video frame from it ;)

AviReader.cs used in some place to get FPS to provide it to DirectShowSource
http://forum.doom9.org/showthread.php?p=774773#post774773
So I waiting for sample AVI files from Mutant_Fruit
http://forum.doom9.org/showthread.php?p=774888#post774888

DirectShow.cs used to try to construct FilterGraph to know - is it possible to open file via DirectShowSource or not. I suggest trying "DirectShowSource(fileName)" is enought now to get a valid diagnostics

dimzon

26th January 2006, 15:49

But keep in mind, I want that YV12 warning message before that..
Can you write which message text I must write for it (my english is poor so provide me this message)?

Doom9

26th January 2006, 16:03

dimzon

26th January 2006, 17:13

0.2.3.2052 26 Jan 2006
Commit by dimzon
-Removed AVIReader (now everything is made via AvsReader)
-Small bugfix opening Script Creator window

dimzon

26th January 2006, 18:23

0.2.3.2053 26 Jan 2006
Commit by dimzon
-AviSynth wrapper now returns information about source colorspace && audio sampletype

dimzon

26th January 2006, 18:25

@Doom9
Dear Doom9, please take look @ AvisynthWrapper.cs && AvsReader.cs
Maybe it's better to merge AviSynthClip && AvsReader to one class (AvsReader)

Inc

26th January 2006, 19:06

0.2.3.2053 26 Jan 2006
Commit by dimzon
-AviSynth wrapper now returns information about source colorspace && audio sampletype

do you mean the dll itself? That was already given ....

typedef struct AVSDLLVideoInfo {

// Video

int width;
int height;
int raten;
int rated;
int aspectn;
int aspectd;
int interlaced_frame;
int top_field_first;
int num_frames;

int pixel_type; // <-----------------

// Audio
int audio_samples_per_second;
int sample_type; // <-----------------
int nchannels;

int num_audio_frames;

int64_t num_audio_samples;
} AVSDLLVideoInfo;

dimzon

26th January 2006, 19:13

do you mean the dll itself? That was already given ....

I mean my avisynthwrapper.dll now provide both before/after conversion. So you can open script forcing RGB24 but will obtain information about original colorspace

Dayvon

26th January 2006, 19:30

Not to spam your thread or anything, but I just wanted to thank you MeGUI dev's. I just got done backing-up/encoding the Band of Brothers series, and your utility worked perfectly. More no hassle rendering than I've done in a long time. Thanks for your hard work, and great GUI!!

Dave

berrinam

26th January 2006, 21:26

Your video source uses the {0} colorspace. Video encoding requires the YV12 colorspace. Add ConvertToYV12() at the end of your AviSynth script and try again.
{0} is the actual colorspace of the video.
What about asking 'Do you want me to fix this up by adding ConvertToYV12() to the end of the script'?

Doom9

26th January 2006, 21:30

well.. I had that part typed out, then I somehow moved away, and once again coming back my text was gone.. thus the shorter message.. yes, having a dialog which would offer the automatic adding would be good, but then we also need the re-opening of the source

Inc

27th January 2006, 09:18

I mean my avisynthwrapper.dll now provide both before/after conversion. So you can open script forcing RGB24 but will obtain information about original colorspace

? That was alreay done as inf.pixel_type was taken from the original frame before forcing the conversion to RGB/YUY2/YV12

vi->pixel_type = inf.pixel_type

And the outgoing Cspace is determined by the Cspace requested in the avs_getvframe(....,....,"RGB") command.

I do think we should merge the project AvsRedirect and AvsWrapper as its getting confusing. If you have questions about whats integrated and what could be done better, lets do it together. ;)

dimzon

27th January 2006, 11:45

What about asking 'Do you want me to fix this up by adding ConvertToYV12() to the end of the script'?

How does you want to patch such script

blablabla
blablabla
blablabla
return last
blablabla
blablabla
blablabla
blablabla

I propose a little another method. Let's create additional avs @ same path:
filename = <originalFileName>.ConvertToYV12.avs

Import(<originalFileName>).ConvertToYV12()

we can do it automatically for every script ;) (even without colorspace checking)

dimzon

27th January 2006, 11:48

? That was alreay done as inf.pixel_type was taken from the original frame before forcing the conversion to RGB/YUY2/YV12

vi->pixel_type = inf.pixel_type

And the outgoing Cspace is determined by the Cspace requested in the avs_getvframe(....,....,"RGB") command.

avisynthWrapepr returns both - original && forced pixel type

I do think we should merge the project AvsRedirect and AvsWrapper as its getting confusing. If you have questions about whats integrated and what could be done better, lets do it together. ;)
No. I have branch it to avoid any backward-compatibility support. It must work only with complimentary AvisynthWrapper.cs. You can use it's source code to merge parts back to AVSRedirect throught...

Inc

27th January 2006, 11:54

No. ...
You can use it's source code to merge parts back to AVSRedirect throught...
The source code, and main add ons are from new_avsRedirect.dll ;) So no need to merge things "back" ;)

however ....

berrinam

27th January 2006, 12:31

How does you want to patch such script

blablabla
blablabla
blablabla
return last
blablabla
blablabla
blablabla
blablabla

I see that a script like that could be a problem, but who really does that? There's not any point in writing return last in a script. If everything is in variables, then what you would do is just write the line "video", or whatever your variable-name is. The only appropriate place for return is in AviSynth functions. Also, why would you have more text after you have returned the video?

I propose a little another method. Let's create additional avs @ same path:
filename = <originalFileName>.ConvertToYV12.avs

Import(<originalFileName>).ConvertToYV12()
Is this really necessary? In my local implementation, it just appends ConvertToYV12() to the end of the script. It then checks if the modified script is fine, and if not, it tells the user, who can choose whether to abort. I agree, your method is guaranteed to work and might be a more elegant solution because of this, but:
1. It creates an extra file to be deleted, and the unexpecting user may be surprised to find two avisynth files where there should only be one. Which one should he/she choose?
2. It requires more code restructuring to handle the fact that there is ANOTHER file.
3. As I said above, that situation should never really arise, and my implementation will abort gracefully in this situation: telling the user that appending converttoyv12 didn't work, and giving the user the option of aborting.

People who really know a lot about scripting will almost definitely not want the quick fix ConvertToYV12() at the end of the script, because, for most sources, it should have been YV12 in the first place, and so they will want to work out where the non-YV12 colorspace came from. I think it is only these people who will be writing the scripts with return last in them.

we can do it automatically for every script ;) (even without colorspace checking)I think that shouldn't be done, because:
1. It adds additional pointless filters. Why slow the encoding down (even though not by much).
2. It will hide problems that the experienced AviSynth user will want to know about. If there is a conversion to RGB, these people won't want it hidden by a ConvertToYV12 at the end, because colorspace conversions cause quality loss.

Never-the-less, none of these points actually give much reason against either of our implementations, so let's see what Doom9 says.

dimzon

27th January 2006, 12:36

0.2.3.2054 27 Jan 2006
Commit by dimzon
-SourceDetector doesn't create temporaly AVS anymore

dimzon

27th January 2006, 12:41

I see that a script like that could be a problem, but who really does that?
I do ;)
Actually I usereturn last instead of commenting/removing rest lines of script. Such trick (return last) is not a good programming style for regular code but avisynth scripts is mostly temporaly (just encode and forget) so i prerer this way bcz it is faster ;)

berrinam

27th January 2006, 13:14

I still don't like this return last. As you said, it's not good programming style. Also, would you seriously just let MeGUI do your ConvertToYV12 wrapping? If you found out it isn't YV12, wouldn't you go: "Hang on a minute, what's up? My source should be YV12, so where's it been converted away from YV12? I better fix it up"

Ok, how about this: I'm just about to commit my version, which manages the "don't ask me again" dialogs as well as all the error messages. If Doom9 wants your way, then you just change the ConvertToYV12 section of the code. There are two places you should look at: JobUtil.prepareVideoJob, which calls the function VideoUtil.checkVideo(string)

berrinam

27th January 2006, 13:40

dimzon

27th January 2006, 13:48

0.2.3.2055 27 Jan 2006
Commit by berrinam:
-Added video input checking (video exists, has no errors, colorspace==yv12, mod16) and warns the user
-Refactored dialogs, added MessageBoxEx reference, and removed MPEG2SourceChoice
-Removed OneClickDefaults.cs, OneClickDefaultWindow.cs/.resx

AvsReader avi = AvsReader.OpenScriptFile(avsFile);
if (!avi.Clip.HasVideo)
return "AviSynth script has no video";
Take look @ AvsReader.OpenScriptFile first:
_enviroment = new AviSynthScriptEnvironment();
_clip = parse? _enviroment.ParseScript(script, AviSynthColorspace.RGB24) : _enviroment.OpenScriptFile(script, AviSynthColorspace.RGB24);
if (!_clip.HasVideo)
throw new ArgumentException("Script doesn't contain video");

Doom9

27th January 2006, 13:55

If Doom9 wants your way, then you just change the ConvertToYV12 section of the code. There are two places you should look at: JobUtil.prepareVideoJob, which calls the function VideoUtil.checkVideo(string)I don't think such breakout scripts should be of our concern.. those who write advanced scripts should be knowledgeable enough to figure out what can go wrong.. our main concern has to be making things work for the users that have very little or no AviSynth knowledge at all.

@dimzon: why do you use _ prefixes for variable names when the rest of the project uses no such thing? It's breaking with style. I generally like to use the java style notation (somevariablename becomes someVariableName.. first letter small, subsequent letters at positions where a word could be separated in capital letters). I know I'm not being consistent but I think considering that so many people are working on this now, we really should strive to be consistent throughout the code. The same goes for comments.. every method should have one (once again I've been too lazy in the past), and I'm also no fan of the whole if something ? then : else notation.. it makes a program harder to read, and forces you to read the entire line, then hammer out the logic in your head rather than just read over the if and know what it's all about.

dimzon

27th January 2006, 14:31

why do you use _ prefixes for variable names when the rest of the project uses no such thing?
first of all - unfortunally MeGUI project doesn't have consustent style across it' sources at all
second - AFAIK Java has not fixed (only one style applicable) notation
third - I'm using Microsoft-proposed style.
this means:
all nonpublic fields must start with _ (private int _someInt)
all nonpublic methods must have lowercase fiirst char (private int someInt() )
all public methods must have upper case first char (public int SomeInt() )

its really usefull notation: looking @ such code

_script = generateScript(val);
Start();

You can say:
_script is protected field, Start() is public and generateScript is protected method without looking @ definition.

But if You don't like this stye feel free to change it using Refactoring features of VS2005 (right click on name and choose Refactor->Rename)

The same goes for comments.. every method should have one
It's my fault. Unfortunally my english is poor so I believe reading source will give you better infotmation against reading comments.

and I'm also no fan of the whole if something ? then : else notation.. it makes a program harder to read, and forces you to read the entire line, then hammer out the logic in your head rather than just read over the if and know what it's all about.
Agreed, sometimes it's harder to read then if/else...

Doom9

27th January 2006, 14:43

all nonpublic fields must start with _ (private int _someInt)But Microsoft also suggests the use of properties for publicly accessible variables.. thus you can use what you're using for methods and transfer it directly to variables. Any variablename starting with a lowercase letter is private, anything that's exposed is a property which has the same name but starts with a capital letter. I think I've been following that style pretty closely where variables are concerned.

However, I don't quite see the necessity to see the difference.. if you're working "inside" the class.. you don't really care whether a method is public or not. If you're working "outside" the class.. if it's private you won't even get to see it.. plus then there are the protected and internal intricacies that you can cover with changing just one letter. If working "outside" a class, if intellisense doesn't pick up a method name you know is there.. you have to go back and change the access modifier anyway.

Come to think of it.. iirc most samples I've seen using the _ notation were done by programmers coming from a language that doesn't have properties and doesn't commonly work with setters/getters. I'm not used to seeing _ in variable names in msdn samples (for C# of course)

dimzon

27th January 2006, 15:31

if you're working "inside" the class.. you don't really care whether a method is public or not.
When I'm looking @ ugly code insude class can say - ok, this method use only nonpublic features, I can refactor it ;)

Come to think of it.. iirc most samples I've seen using the _ notation were done by programmers coming from a language that doesn't have properties and doesn't commonly work with setters/getters. I'm not used to seeing _ in variable names in msdn samples (for C# of course)
http://www.akadia.com/services/naming_conventions.html
http://dotnet.mvps.org/dotnet/faqs/?id=namingconventions&lang=en
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/cpgenref/html/cpconnamingguidelines.asp

dimzon

27th January 2006, 15:46

added MessageBoxEx reference
What is MessageBoxExLib.dll ?
Where I can find it's source code?

Doom9

27th January 2006, 15:46

Your last link, the official guidelines is very enlightening. It suggests the use of the camel notation for protected fields and method parameters, and discourages the use of public fields (instead it suggest properties just like I've been using, and the use of the pascal notation)
Basically the only difference is in terms of method names, where I've always been using camel notation (which is how I was taught to code in Java).
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/cpgenref/html/cpconcapitalizationstyles.asp[

When I'm looking @ ugly code insude class can say - ok, this method use only nonpublic features, I can refactor it What do we have refactor tools for? So that they help when you refactor a method that could have an influence on others ;) I saw nothing on the MSDN .NET coding guidelines about prepending _ for private variables and use of capital letters for methods only if they're public. Since those are guidelines for creating class libraries, it wouldn't make a whole lot of sense to come up with guidelines for how you handle things internally.

I don't mind much using capitalization to differ between exposed and non exposed methods, but it's already inconsistent (the guidelines suggest to use camel notation for protected variables.. versus pascal for all (and thus also protected) methods).
The articles you linked to also contradict each other (e.g. Microsoft says no to Hungarian notation, http://www.akadia.com/services/naming_conventions.html suggests it for protected variables). Since IDEs tell you what type a variable is if you hover over it, and also tell you once you start typing and get to chose from the available variables, I don't see the point of hungarian notation at all, and they even use it inconsistently (no hungarian for OleDBConnection? and then there are probably a hundred more exceptions).

dimzon

27th January 2006, 15:53

dimzon

27th January 2006, 16:07

Yet another reason why i prefer underscope for nonpublic fields:

private string avsScript;

public Init(string avsScript)
{
avsScript = avsScript; // won't work
this.avsScript = avsScript; // will work;
}

compare it to:

private string _avsScript;

public Init(string avsScript)
{
_avsScript = avsScript; // will work
this._avsScript = avsScript; // will work;
}

and when you looking @ method implementation you can't tell exatly - is you accessing to methid argument or private field until you scroll up && look @ method definition

Doom9

27th January 2006, 16:14

why on earth would it matter whether this.avsScript is protected or private (keep in mind.. MS suggests avsScript in case of a protected class variable). _variable looks butt-ugly.. I don't consider myself to put much weight on appearances but I do seem to have a strong opposition to code I think looks ugly.

what is Your desision about it (seem's like MeGUI full wins 3:1 ) I still like the stripped down edition.. it was created for a reason and that's getting people on the hook even if they're just looking for a tool do to video encoding.. then you offer the easy upgrade path to a much more extensive functionality so it's easier to get the cautious people onboard.

ChronoCross

27th January 2006, 16:38

If I remember correctly the _privateVariable was originally implemented in C. it's the style that most of the older professors I take use. it's what I've been taught to use for C. But for java and C++ I simply use lowercase of the first word uppercase of the second.

Doom9

27th January 2006, 16:53

How about this:
Use properties for all variables that are externally exposed (including protected, unlike what MS says - they suggest camel notation which is inconsistent with the suggestion of pascal for all exposed methods), and use pascal notation for those (as MS suggests for properties). Use pascal notation for all publicly exposed methods (as MS suggests). Use camel notation for any non exposed method and variables. That's fully consistent and pretty.

So we'd have

private int myPublicExposedVariable, myProtectedExposedVariable2, myPrivateVariable;
protected MyPublicExposedVariable{...}
public MyProtectedExposedVariable{..}

public MyMethod(int someParameter)
{
this.myPrivateVariable = someParameter;
}
myPrivateMethod(int myPrivateVariable)
{
this.myPrivateVariable = myPrivateVariable;
}
I don't think this.myPrivateVariable = myPrivateVariable is a problem... on the contrary.. it forces you to think a little when you name method parameters the same as class variables.

dimzon

27th January 2006, 17:07

I don't think this.myPrivateVariable = myPrivateVariable is a problem... on the contrary.. it forces you to think a little when you name method parameters the same as class variables.
this will works while you are writing code from the scratch. But if You are changing non your code or refactoring it it's easy to make a mistake...
Thats why I strongly prefer _ pefix for private fields (please, stop name it variables, it's not variables but fields)

dimzon

27th January 2006, 17:09

@By the way - seems like we have something like flamewar now ;)
Maybe we will stop it (as I said before it's esy to convert to another notation using VS2005 refactoring->rename feature) and switch to more meaningfull topic (ie - what to do next)

Doom9

27th January 2006, 17:30

as I said before it's esy to convert to another notation using VS2005 refactoring->rename featureBut there's no "switch to approved notation" refactoring so each variable will have to be changed.. so it's important we stop using different notations. The fact that large parts of the code were written by myself make this rather easy though.. it would be a major waste of time to change things.. so consider my examples above as the howto for any further changes. berrinam, who also added his own classes, also stuck to the same notation as I've been using as far as I can see.

And by the way, if you add an element in Visual Studio.. it's default accessor is private and it uses camel notation.. no _. So the _ prefixes are most definitely out.

In terms of where to go.. we need an as-is analysis.. compile a list of what we have in terms of job management and processing, and then design to to-be scenario. One crucial point will certainly be the mapping between the IDictionary containing the jobs and the GUI.

dimzon

27th January 2006, 18:01

But there's no "switch to doom9 approved notation" refactoring so each variable will have to be changed.. so it's important we stop using different notations.
Ok, throught I still doen't like private fields without _ ...

One crucial point will certainly be the mapping between the IDictionary containing the jobs and the GUI.
take look @ ListView.VirtualMode property - it's fine

By the way - I have small (approx 4 kb each) 16*16 icons - we can show them in Queue (red for error, green for complete etc)

Doom9

27th January 2006, 18:15

By the way - I have small (approx 4 kb each) 16*16 icons - we can show them in Queue (red for error, green for complete etc)Sounds like a good idea.. I don't think they can replace the status string but perhaps if added at the beginning they can give a quicker overview.

stax76

27th January 2006, 18:19

Every dev has his own style so the project initiator is in charge of enforcing a style. IIRC MS recommends camel case for fields but on the same time they recommend to access them using 'this' because otherwise there would be no destinction between fields and params.

public void SomeMethod()
{
m_someField = null; // IIRC ungarian notation / popular for C++, obsolete for .NET
_someField = null; // popular but like too many abbreviations not recommend for .NET, .NET code should be easy to read and look nice
someField = null; // not good because params use it too, IIRC used by SharpDevelop
SomeField = null; // my style, properties use PropName/PropNameValue pattern, IIRC MS code generators use it too!
this.someField = null; // recommend by MS but not very popular
}

dimzon

27th January 2006, 18:38

Sounds like a good idea.. I don't think they can replace the status string but perhaps if added at the beginning they can give a quicker overview.
http://www.mytempdir.com/413057

ChronoCross

27th January 2006, 18:41

this.someField is required when using Python. It's basically the main syntax when building classes. Else alot of the code won't work. but for a majority of people programming actual apps this isn't necessary cause python isn't used by companies.

Doom9

27th January 2006, 19:23

this.someField = null; // recommend by MS but not very popularI've started using it as well. In VS2k3, when you start typing, it doesn't show you local and global variables, hence I had to resort to typing this. to get a list of globals.. this is no longer the case with VS2k5 though.. it shows all the variables right away.
By the way, where the heck does "field" come from? I learned about methods and variables in college (and prior to OO programming, functions in Pascal in high school)

dimzon

27th January 2006, 19:29

By the way, where the heck does "field" come from? I learned about methods and variables in college (and prior to OO programming, functions in Pascal in high school)
this is oop term. You say structure fields, not structure variables, isn't it. Ad object is struct + methods...

dimzon

27th January 2006, 19:42

@Doom9
Why not to talk via ICQ?

Richard Berg

27th January 2006, 19:52

@Doom9 -- field : variable :: method : function

I hate _field personally. I only use _ for parameters to constructors:

public class MyClass
{
private int x, y;

// don't feel like making up another name (xParam? myX? xInput?)
// so I use _x
public MyClass(int _x)
{
this.x = _x;
}
public MyClass(int _x, int _y)
: this((_x + _y) / 2)
}

...and even that is just a C++ trick that's stuck with me...not condoned by the other C# guys I work with. (Initializer lists aren't as powerful in C#, though they're certainly "better" from an OO standpoint).

Everywhere else, _ is banished like ALLCAPS; both remind me of C/Win32 (ugh).

berrinam

27th January 2006, 23:16

Video cutting in the AviSynth creator for removal of ads, etc
Description: Since video and audio can be done through AviSynth cutting can be completely precise. Use should be done using the trim statement.
Status: No-one is working on this yet.I think Doom9 had some plans about this. Am I right, and if so, what were/are they?

Dayvon

28th January 2006, 00:46

Hey guys...

I was messing around in Paint Shop the other day, and I decided to make an icon for MeGUI. I use this dock thing and the default icon looks pretty boring so I made this thing up. I figured I'd let you guys have at it if you want, if not, then by all means it'll just be my personal MeGUI icon.

If you do like it however, you're totally free to use it. The fonts are freeware fonts, and the I have a higher resolution version if you want it. I tried to run with the same colors as Gordian Knot just for familiarities sake.

Anyway, I can't do code developing, but I can do this. So let me know what you think...
http://church.crossingatwoodland.com/ChurchFiles/SermonText/MeGUI2Icon.png