[MPlayer-dev-eng] [PATCH] Fix capture width/height selection in tvi_dshow

[Reimar Döffinger]

On Sat, Mar 02, 2013 at 10:46:09AM +0700, Vladimir Voroshilov wrote:
> +    case TVI_CONTROL_VID_SET_WIDTH_HEIGHT:
> +	{
> +	    VIDEO_STREAM_CONFIG_CAPS *pCaps;
> +	    VIDEOINFOHEADER *Vhdr;
> +            int width = ((int*)arg)[0];
> +   	    int height = ((int*)arg)[1];
> +            int i;
> +	    if (priv->state)
> +		return TVI_CONTROL_FALSE;
> +
> +            if(!priv->chains[0]->arpmt)
> +                return TVI_CONTROL_FALSE;
> +            /* Loop over all detected formats... */
> +	    for (i = 0; priv->chains[0]->arpmt[i]; i++) {
> +                /* same FourCC as previously specified */
> +		if (!check_video_format
> +		    (priv->chains[0]->arpmt[i], priv->fcc, width, height))
> +                    continue;
> +	        pCaps = priv->chains[0]->arStreamCaps[i];
> +   	        if (!pCaps)
> +		    continue;
> +
> +                /* requested width fits to min/max range */
> +                if (width < pCaps->MinOutputSize.cx
> +		    || width > pCaps->MaxOutputSize.cx)
> +		    continue;
> +
> +                /* requested height fits to min/max range */
> +	        if (height < pCaps->MinOutputSize.cy
> +		    || height > pCaps->MaxOutputSize.cy)
> +		    continue;
> +
> +                /* switch format if found suitable one */
> +                priv->chains[0]->nFormatUsed = i;
> +                break;
> +            }

I think the behaviour would be more consistent if this was a function
and also used from SET_WIDTH and SET_HEIGHT, in the way that
SET_WIDTH would call it with (new width, old height)

> +	    pCaps = priv->chains[0]->arStreamCaps[priv->chains[0]->nFormatUsed];
> +	    if (!pCaps)
> +		return TVI_CONTROL_FALSE;
> +	    if (width < pCaps->MinOutputSize.cx
> +		|| width > pCaps->MaxOutputSize.cx)
> +		return TVI_CONTROL_FALSE;
> +	    if (height < pCaps->MinOutputSize.cy
> +		|| height > pCaps->MaxOutputSize.cy)
> +		return TVI_CONTROL_FALSE;
> +
> +	    if (width % pCaps->OutputGranularityX)
> +		return TVI_CONTROL_FALSE;
> +	    if (height % pCaps->OutputGranularityY)
> +		return TVI_CONTROL_FALSE;
> +
> +	    if (priv->chains[0]->pmt)
> +		DeleteMediaType(priv->chains[0]->pmt);
> +	    priv->chains[0]->pmt =
> +		CreateMediaType(priv->chains[0]->arpmt[priv->chains[0]->nFormatUsed]);
> +	    DisplayMediaType("VID_SET_WIDTH_HEIGHT", priv->chains[0]->pmt);
> +
> +	    if (!priv->chains[0]->pmt || !priv->chains[0]->pmt->pbFormat)
> +		    return TVI_CONTROL_FALSE;
> +	    Vhdr = (VIDEOINFOHEADER *) priv->chains[0]->pmt->pbFormat;
> +	    Vhdr->bmiHeader.biWidth = width;
> +	    if (Vhdr->bmiHeader.biHeight < 0)
> +		    Vhdr->bmiHeader.biHeight = -height;
> +	    else
> +		    Vhdr->bmiHeader.biHeight = height;
> +	    priv->chains[0]->pmt->lSampleSize = Vhdr->bmiHeader.biSizeImage =
> +		    labs(Vhdr->bmiHeader.biBitCount * Vhdr->bmiHeader.biWidth *
> +		    Vhdr->bmiHeader.biHeight) >> 3;
> +
> +	    priv->width = width;
> +	    priv->height = height;

You can probably even put all of this code into that function, and
thus avoid having things like the Min/Max size validation duplicated
between those different SET_ functions.
Or alternatively, if the SET_WIDTH and SET_HEIGHT are actually
not used at all any more after this change (I didn't double-check
how all this works) then just removing them might be an option, too.
Still, I am a fan of using lots of functions, they tend to make
the code much easier to understand.