Fix handling of per-renderer aspect-ratio settings #98

wants to merge 1 commit into


None yet

3 participants

ExSport commented Dec 10, 2012

Fix for CustomMencoderOptions ignored (on rendered level):


I think the proposed fix is incorrect.

The code now does exactly what the comment above it says:

    if (isNotBlank(rendererMencoderOptions)) {
        * ignore the renderer's custom MEncoder options if a) we're streaming a DVD (i.e. via dvd://)
        * or b) the renderer's MEncoder options contain overscan settings (those are handled
        * separately)

        // XXX we should weed out the unused/unwanted settings and keep the rest
        // (see sanitizeArgs()) rather than ignoring the options entirely
        if (rendererMencoderOptions.contains("expand=") || dvd) {
            rendererMencoderOptions = null;

The overscan settings are handled separately, that is why "expand=" should be weeded out. However, the XXX comments also state that the code could use improving; clearing out all options (instead of just the one handled elsewhere) is a bit of overkill.

With the current code, DVDs always get their options cleaned, regardless of the custom options. With the proposed fix, they only get it cleaned if the custom options contain "expand=". This sounds wrong to me.


If expand is present for DVD, picture is broken so for DVD cleaning expand is correct. When overscan feature is enabled, it is handled separately so cleaning expand in CustomMencoder setting is wrong (I understand it in this way)
So for my understanding changing || to && should be correct.
When overscan is enabled, it is "maintained with another code" which take care of another already existing expand (e.g. in custommencoderoptions) so it can coexist with each together and makes no conflicts.
Removing "expand" in CustomMencoderOptions will totally break usage of this added feature because it is mainly used for fixing AR of many TV renderers so for any other (PS3,...) it should not be used (like when used in GUI which is applied to all renderers)
Or am I wrong? I don't think so because this code I made one and half year ago? and it worked well until some day when code was changed...Ok, found your fix 👍 Code had bug when "CustomMencoderOptions =" property was defined but empty. You fixed it here:
But unfortunately you rewrote && to || which seems to be wrong for me:
((params.mediaRenderer.getCustomMencoderOptions()!=null && !(params.mediaRenderer.getCustomMencoderOptions().indexOf("expand=") != -1 && dvd))

You are correct in that the old code appears to read different from the new code. However, I don't know whether that was intentional or not, given the comments above it.

I'll leave this one to @chocolateboy.

ExSport commented Dec 26, 2012

Now I see I switched you with Chocolateboy who made this fix, sorry for that. Hope it wasn't intentional so CustomMencoderOptions will be back again 👍

@chocolateboy chocolateboy was assigned May 17, 2013


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment