Skip to main content

impl/havi/port/mpe/HDVideoDevice.java, is this right?

4 replies [Last post]
david_crandall
Offline
Joined: 2010-01-05
Points: 0

I don't fully know the intention behind this, but it seemed...interesting... to me.

Just so we know what types we're dealing with.

<br />
    private int supportedDFCs[] = {VideoFormatControl.DFC_PROCESSING_UNKNOWN};<br />

Lines 229-272:
<br />
            if(isBackgroundVideo())<br />
            {<br />
                supportedDFCs = mediaApi.getSupportedDFCs(nDevice);<br />
                if (Logging.LOGGING)<br />
                {<br />
                    log.debug("background device - supported DFCs: " + Arrays.toString(supportedDFCs));<br />
                }<br />
            }<br />
            int config[] = HDScreen.nGetDeviceConfigs(nDevice);<br />
            java.util.Vector v = new java.util.Vector();<br />
            for(int i = 0; i < config.length; ++i)<br />
            {<br />
                // TODO_DS, TODO:  equals is incorrect, needs DFC and persistence<br />
                if (config[i] == nNotContrib)<br />
                {<br />
                    notContributing = new HDVideoConfiguration(this, config[i]);<br />
                }<br />
                else<br />
                {<br />
                    for(int j = 0; j < supportedDFCs.length; j++ )<br />
                    {<br />
                        int dfc = supportedDFCs[j];<br />
                        int defaultDfc = VideoFormatControl.DFC_PROCESSING_NONE;<br />
                        if( dfc == VideoFormatControl.DFC_PROCESSING_UNKNOWN)<br />
                        {<br />
                            defaultDfc = VideoFormatControl.DFC_PROCESSING_UNKNOWN;<br />
                        }<br />
                        HDVideoConfiguration cfg = new HDVideoConfiguration(this, config[i], dfc);<br />
                        v.addElement(cfg);</p>
<p>                        // TODO, TODO_DS: use persistence to get current config and DFC and use .equals<br />
                        if (defaultConfiguration == null && cfg.equals(curr, defaultDfc))<br />
                        {<br />
                            // Save current/default configuration<br />
                            defaultConfiguration<br />
                            = currentConfiguration = cfg;<br />
                            if (Logging.LOGGING)<br />
                            {<br />
                                log.debug("current config updated to: " + cfg);<br />
                            }<br />
                        }<br />
                    }<br />
                }<br />
            }<br />

For line:
<br />
                        if (defaultConfiguration == null && cfg.equals(curr, defaultDfc))<br />

Did you mean?
<br />
                        if (defaultConfiguration == null && cfg.equals(curr, dfc))<br />

Because, as I see it, defaultDfc is either VideoFormatControl.DFC_PROCESSING_UNKNOWN or VideoFormatControl.DFC_PROCESSING_NONE. So it seems odd that we would be looking in our configuration for something that's one of those two, and assigning it.

Now, one of the caveats here, is that I'm not intimately familiar with this, so there might be something I'm not aware of, hence why I'm asking.

Message was edited by: david_crandall

Message was edited by: david_crandall

Reply viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.
david_crandall
Offline
Joined: 2010-01-05
Points: 0

I redid/undid the change as suggested, and I couldn't get the original error I was getting to cause me to look at this section of code, to happen. So I'm assuming that this was an error precipitated by something else that has since been solved.

Thank you very much for looking.

scottdeboy
Offline
Joined: 2009-02-02
Points: 0

Hi David

I will take a closer look at this code tomorrow and let you know if there's a bug here.

Thanks
Scott

scottdeboy
Offline
Joined: 2009-02-02
Points: 0

I can't find spec references relevant to assignment of the default configuration, so I'm basing my review on the HVideoDevice.getDefaultConfiguration() javadoc and the function comments for mpeos_dispGetSupportedDFCs(mpe_DispDevice decoder, mpe_DispDfcAction** dfcs) in mpeos_disp.h (included below).

It seems reasonable to expect a 'default' configuration to have -no- dfc applied. I believe the current code is behaving correctly by ensuring the default configuration's dfc value is set to NONE if NONE is in the supportedDfcs array and UNKNOWN otherwise.

Regarding the potential code change:

HDVideoConfiguration passes equals calls to UniqueConfigId (which is assigned in the HDVideoConfiguration constructor). UniqueConfigId holds a reference to the handle and the dfc passed in the HDVideoConfiguration constructor.

If we changed the call to HDVideoConfiguration.equals to take dfc instead of defaultDfc, UniqueConfigId.equals would be comparing its dfc value against the passed-in dfc value (both 'dfc'), and always return true for the dfc portion of the equals evaluation.

The change would result in a default configuration with a dfc equal to supportedDfcs[0], which may or may not be NONE or UNKNOWN, and I don't think that is the intended behavior.

The code is a bit confusing and is missing some helpful comments. I will make a few changes to the code and continue to look for relevant spec references.

HVideoDevice.getDefaultConfiguration():

/**
* Returns the default HVideoConfiguration associated with this
* HVideoDevice. This (single) default configuration should correspond to
* some well-behaved settings for the device, such as, a minimal
* configuration or factory preset settings.
*
* @return the default HVideoConfiguration of this HVideoDevice.
*
* @see HVideoConfiguration
*/

mpe_Error mpeos_dispGetSupportedDFCs(mpe_DispDevice decoder, mpe_DispDfcAction** dfcs):

/**
* Get the supported Decoder Format Conversions (DFC) for the given decoder
*
* @param decoder is the decoder
* @param dfcs is a array of DFCs supported by this decorder. Array is always terminated by
* MPE_DFC_PROCESSING_UNKNOWN.
*
* @return error level
*/

david_crandall
Offline
Joined: 2010-01-05
Points: 0

Much thanks for looking into this. I can't remember the initial exception that was causing me to look at this piece of code, but, I'll reverse what I did and post the results on the week of the 10th.