MeGUI development

[Doom9]

For example (as said before), if i choose High profile, then enable all the macroblock options (such as adaptive DCT) and then i change to main profile and back to high profile, those boxes are no longer checked.

Then who the heck broke that? It all worked nicely when I released my last build. So in the meantime somebody broke the functionality and all the confusion stems from that.

So let me say it once and for all: for the x264 configuration dialog, if an option is not to be accessible, it keeps its current value, but it's disabled. The rest is done in the x264TristateAdjustment method in the CommandlineGenerator class. It's rather frustrating for me to having spent hours to get this darned thing working only to have it broken again by somebody else

[Sharktooth]

i think the behaviour was modified when the whole tri-state was rewritten.
i've a local copy of the old method. i can restore it in few minutes (i'll do it tonight).

[godhead]

berrinam, go ahead and put me down for the following, since I already know the MP4 muxer and commandlinegenerator code from my work on track names.

Add support for the -sbr or -sbrx tags in mp4box commandline
Description: Add a checkbox in MeGUI MP4 Muxer which says something like "AAC is SBR", so that the audio information is correctly stored in MP4
Status : No-one is working on it

[Doom9]

Add support for the -sbr or -sbrx tags in mp4box commandline

Please do not forget that this involves the automatic setting of that checkbox in auto-mode / one click mode when audio is being encoded to aac. That makes it a bit less than trivial

[Richard Berg]

Isn't a disabled control essentially not playing any role?

No. A control that's disabled but checked means "I'm enabled and there's nothing you can do about it." The most common example of this state comes from installers. Typically you can check or uncheck optional components, but components required to make the program work will be checked-and-disabled.

[stax76]

No. A control that's disabled but checked means "I'm enabled and there's nothing you can do about it." The most common example of this state comes from installers. Typically you can check or uncheck optional components, but components required to make the program work will be checked-and-disabled.

You are both right, it depends on the case e.g. is it a child etc., a disabled control can also mean it has no relevance.

[berrinam]

Originally Posted by Doom9

Please do not forget that this involves the automatic setting of that checkbox in auto-mode / one click mode when audio is being encoded to aac. That makes it a bit less than trivial

Actually, it turns out this isn't necessary. As far as I can see, all aac encoding by MeGUI is put straight into the mp4 container, and -sbr signalling isn't required with mp4 input, because the info is already there. This option is only really useful for people who are using externally encoded aac files.

[Doom9]

This option is only really useful for people who are using externally encoded aac files.

Hmm, you're right. But it's something to be kept in mind if the winamp aac encoder is ever to be supported.

[godhead]

Originally Posted by Doom9

Please do not forget that this involves the automatic setting of that checkbox in auto-mode / one click mode when audio is being encoded to aac. That makes it a bit less than trivial

Noted.

[Richard Berg]

What's the best way to generate a patch so it's easier for you to accept submissions?

[berrinam]

Originally Posted by godhead

Noted.

godhead, it's not necessary to do that. See my post just below Doom9's.

[dimzon]

@all dev's
Hi!
I was @ new year vacation and I spend this time with my family mostly far from PC. But I found time to take closer look @ MeGUI sources. And I'm frustrating now!
I found some amount of non-optimal code architecture, including annoyng copy/paste programming paradigm... It's a really not good!
I really don't understand some decisions in it!
Take look @ audio-related staff. Why all configuration forms are created by copy/paste metodology? Hey, forms is objects and you can still use inheritance/poliphormysm with it! I really don't understand why does You use ugly ENUM<->Int casting, why not just use enums?
In other case I don't understand unnecesarry using of "Property Paradigm":

int someProperty;
public int SomeProperty
{
get{return someProperty;}
set{somePropert=value;}
}

why not use just fields to decrease code size:

public int SomeProperty;

(don't forget - You can switch from field to property @ first request in 1 minute!)

I believe we must stop implementing any additional features until full code review and refactoring!

[Doom9]

1) Forms: Please feel free to lay down a good foundation for GUI inheritance.. I only have limited experience with it, hence I implemented them all as separate forms but I do agree that it would make more sense and be easier to maintain to have basis forms and forms that derive from it. The same obviously also goes for the video config, muxer and settings forms. So when you start working at the audio, please do lay the groundwork for a major improvement in this area.

2) enums <-> ints... once again plain ugly and my fault.. I'm just not sure if you can bind enums to dropdowns.. I guess that's why I started with ints, then figured for code maintainability it would be better if the names of the options were spelled out, hence the enums. You'll see that in more recent codes, there are no such conversions anymore and I'm using the enums directly, as it should be. Once again, if you start working on the audio, please feel free to change that.

3) There however I have to disagree with you. Variables should not be declared public. In Java we have setters and getters to change/get variables, in .NET we have properties. In VS2k5 you can easily define your private variable, then using the refactoring functionality turn this into a property with about two clicks (I don't recall the exact procedure.. it's been a while since I watched that "what's new" video). And in both Java classes at university as well as coding practices for C# they tell you that accessing variables directly is bad style of programming. For most variables, you don't need any accessors because they are only used within the class where they are declared, but those that are to be accessed externally should use properties.

[berrinam]

Originally Posted by dimzon

I believe we must stop implementing any additional features until full code review and refactoring!

Although I agree with you, and it certainly would be nice to see the code more organised, this would mean a complete halt to development for a considerable time period. Could this code clean-up not be done gradually?

[dimzon]

Originally Posted by Doom9

3) There however I have to disagree with you. Variables should not be declared public. In Java we have setters and getters to change/get variables, in .NET we have properties. In VS2k5 you can easily define your private variable, then using the refactoring functionality turn this into a property with about two clicks (I don't recall the exact procedure.. it's been a while since I watched that "what's new" video). And in both Java classes at university as well as coding practices for C# they tell you that accessing variables directly is bad style of programming. For most variables, you don't need any accessors because they are only used within the class where they are declared, but those that are to be accessed externally should use properties.

C# is not Java. "Property Paradigm" is good for java @ early development stage - if you decide to add some logic @ properties later you does not need to replace direct field access by getter/setter call. But in C# it looks same:

obj.SomeFileld = value;
obj.SomeProperty = value;

So you can easy convert public field to property without massive codechange.
Until you does not expose your objects from MeGUI it's safe to use public fields instead of primitive public properties!

[dimzon]

Originally Posted by berrinam

this would mean a complete halt to development for a considerable time period

Definitly YES! Code structure will be significally changed during cleanup/refactoring. It's impossible to do such massive changes without halting development... I'm afraid everything must be altered and, maybe, backward compatiblity (saved jobs and profiles) will be broken...

Now let's discuss about MeGUI architecture.
I'm strongly prefer to keep main MeGUI architecture as simple as possible. As fact megui is job-based sofware so Job && JobList && JobExecutor is main GUI-less abstract classes. Another abstract classes would be JobProvider - some component wich provide some GUI for concrete job creation, tuning and tweaking and JoblessUtility - some additional visual staff (like bitrate calculator etc.)
AudioJobProvider/VideoJobProvider/MuxJobProvider implementations itself can contain multiple configureable and extensible components (pattern Facade)...
MeGUI must provide JobList management functionality and act as a host for multiple JobProviders and JoblessUtilities. It must provide additional bridge between JobProvider and JoblessUtility (for AviSynth script creation or bitrate calculator).

[Doom9]

Actually, you can use properties as full blown methods... the StatusUpdate contains a few examples, and there are some in other classes as well. For consistency purposes and design, every variable that needs to be available publicly should be wrapped in a property. Anything internal can stay a simple variable. This has a lot of advantages. Even inside the same project, with properties come nice descriptions that show up when using the commandline completion.. so even somebody not quite familiar with a class will be able to use those properties, whereas the use of the internal variables actually requires the study of the source.

JoblessUtility - some additional visual staff (like bitrate calculator etc.)

You're getting a bit overboard here.. it's like you're trying to wrap in a job what isn't one.. I don't think that's a good idea, in fact that makes the whole thing appear stiff and artificial.

Another abstract classes would be JobProvider - some component wich provide some GUI for concrete job creation, tuning and tweaking and

That sounds nice an all and corresponds what I'm currently doing with the video encoding part (an incremental upgrade for the whole project though, first swap out just one part, then the next one, etc), but here's the problem with your approach: There's a great amount of interdependency between a VideoJobProvider and an AudioJobProvider or MuxJobProvider. For instance, if you're using x264 and mencoder and mp4 output, you don't get a single video job, or two or three, but another one that's a muxjob. Things get even more complex when you look at the auto-encoding section, which outputs video, audio and muxjobs at the same time. And then we have the one click mode.. which outputs all kinds of jobs at the same time (well.. almost). Then there's all the job postprocessing after a job.. where does that go? The Main GUI is much more than a VideoJobProvider and AudioJobProvider (and since there's no multiple inheritance in .NET, this already doesn't work and we have to move to interfaces).

I have already done a major refactoring once, redesigned a lot of the classes, and while in the end it's all nicer, it meant a really long time without even so much as a bugfix. Considering how MeGUI has grown, I don't think that's such a good idea. An incremental update, as I have begun to outline, where different parts of the entire project are marked off "under construction" and will be modified while the rest of the program continues to live, is a much more practicable approach.. it also lowers the pressure in having to get things done the right way right now as it will not be possible to make any big changes in the near future.

We can certainly implement many of your suggestions (e.g. JobExecutor, although the name is horrible, the concept is good, same for JobList), but I think it would be better to hammer out the details and do the whole thing in small steps so as to not break everything at once and then having to deal with hundreds of not thousands of compilation errors when trying to put it together, and then the millions of smallish bugs that will have crept in.

[Sharktooth]

Originally Posted by Sharktooth

i think the behaviour was modified when the whole tri-state was rewritten.
i've a local copy of the old method. i can restore it in few minutes (i'll do it tonight).

Done and CVS updated:

0.2.3.2003 10 Jan 2006
Restored x264 tri-state the way it was before the whole tri-state was rewritten to a single method (that implies grayed out controls means also they're disabled in the commandline)

However please do some testing and verify EVERY grayed out control is disabled in commandline

[Doom9]

Restored x264 tri-state the way it was before the whole tri-state was rewritten to a single method

I really wonder what the rewrite contained then? It was meant to just mean the grouping of all this logic in one single method, but apparently somebody completely switched the way things were handled back to the two-state era.

[Sharktooth]

Dont remember who did it, but if you check the changelog it should be there (i think berrinam or charleski)
Also, x264 rev398 has a new switch "--nr <int>" (noise reduction, default value is 0).