[vlc-devel] [PATCH 2/2] v4l2: move more definitions to v4l2_common and factorize for modulators

Rémi Denis-Courmont remi at remlab.net
Tue Mar 13 09:43:05 CET 2012


Le lundi 12 mars 2012 23:24:39 Francois Cartegnie, vous avez écrit :
> ---
> +/**
> + * List of V4L2 chromas were confident enough to use as fallbacks if the
> + * user hasn't provided a --v4l2-chroma value.
> + *
> + * Try YUV chromas first, then RGB little endian and MJPEG as last resort.
> + */
> +static const uint32_t p_chroma_fallbacks[] =
> +{ V4L2_PIX_FMT_YUV420, V4L2_PIX_FMT_YVU420, V4L2_PIX_FMT_YUV422P,
> +  V4L2_PIX_FMT_YUYV, V4L2_PIX_FMT_UYVY, V4L2_PIX_FMT_BGR24,
> +  V4L2_PIX_FMT_BGR32, V4L2_PIX_FMT_MJPEG, V4L2_PIX_FMT_JPEG };

Sharing this with the output makes no sense. For one thing, the output won't 
do JPEG. It should probably use the existing video output fallback chroma 
stuff that Laurent wrote instead.

> +
> +
>  bool get_v4l2pixelformat_by_fourcc( unsigned int *found,
>                                      vlc_fourcc_t i_requested_fourcc )
>  {
> @@ -108,3 +139,383 @@ bool get_fourcc_by_v4l2pixelformat( vlc_fourcc_t
> *i_fourcc, return false;
>  }

These two functions from the previous patch will only clutter the code, as 
either plugins will use either functions. I would really rather have a file 
with only an extern constant FourCC table.

> +/* enumerate capabilities */
> +bool v4l2_enumerate_capabilities( vlc_object_t *p_obj, int i_fd,
> +                                  struct v4l2_capability *cap )
> +{
> +    if( v4l2_ioctl( i_fd, VIDIOC_QUERYCAP, cap ) < 0 )
> +    {
> +        msg_Err( p_obj, "cannot get video capabilities: %m" );
> +        return false;
> +    }
> +
> +    msg_Dbg( p_obj, "device %s using driver %s (version %u.%u.%u) on %s",
> +             cap->card, cap->driver, (cap->version >> 16) & 0xFF,
> +             (cap->version >> 8) & 0xFF, cap->version & 0xFF,
> cap->bus_info ); +    msg_Dbg( p_obj, "the device has the capabilities:
> 0x%08X",
> +             cap->capabilities );
> +    msg_Dbg( p_obj, " (%c) Video Capture, (%c) Video Output, (%c) Audio,"
> +                    " (%c) Tuner, (%c) Modulator, (%c) Radio",
> +             ( cap->capabilities & V4L2_CAP_VIDEO_CAPTURE  ? 'X':' '),
> +             ( cap->capabilities & V4L2_CAP_VIDEO_OUTPUT  ? 'X':' '),
> +             ( cap->capabilities & V4L2_CAP_AUDIO  ? 'X':' '),
> +             ( cap->capabilities & V4L2_CAP_TUNER  ? 'X':' '),
> +             ( cap->capabilities & V4L2_CAP_MODULATOR  ? 'X':' '),
> +             ( cap->capabilities & V4L2_CAP_RADIO  ? 'X':' ') );
> +    msg_Dbg( p_obj, " (%c) Read/Write, (%c) Streaming, (%c) Asynchronous",
> +             ( cap->capabilities & V4L2_CAP_READWRITE ? 'X':' ' ),
> +             ( cap->capabilities & V4L2_CAP_STREAMING ? 'X':' ' ),
> +             ( cap->capabilities & V4L2_CAP_ASYNCIO ? 'X':' ' ) );
> +    return true;
> +}

TBH, I am not sure this should be shared. Again the input and output care 
about different properties.

> +bool v4l2_enumerate_and_set_video_io( vlc_object_t *p_obj, int i_type,
> +                                      int i_fd, unsigned *index )
> +{
> +    int i_ioctl;
> +    struct v4l2_input input;
> +    struct v4l2_input output;
> +
> +    switch( i_type )
> +    {
> +    default:
> +    case V4L2_CAP_VIDEO_CAPTURE:
> +        input.index = 0;
> +        while( v4l2_ioctl( i_fd, VIDIOC_ENUMINPUT, input ) >= 0 )
> +        {
> +            msg_Dbg( p_obj, "video input %u (%s) has type: %s %c",
> +                     input.index, input.name,
> +                     input.type == V4L2_INPUT_TYPE_TUNER
> +                     ? "Tuner adapter" : "External analog input",
> +                     input.index == *index ? '*' : ' ' );
> +            input.index++;
> +        }
> +        i_ioctl = VIDIOC_S_INPUT;
> +        break;
> +
> +    case V4L2_CAP_VIDEO_OUTPUT:
> +        output.index = 0;
> +        while( v4l2_ioctl( i_fd, VIDIOC_ENUMOUTPUT, output ) >= 0 )
> +        {
> +            msg_Dbg( p_obj, "video output %u (%s) has type: %s %c",
> +                     output.index, output.name,
> +                     output.type == V4L2_OUTPUT_TYPE_MODULATOR
> +                     ? "Modulator" : "External analog output",
> +                     output.index == *index ? '*' : ' ' );
> +            output.index++;
> +        }
> +        i_ioctl = VIDIOC_S_OUTPUT;
> +        break;
> +    }
> +
> +    /* Select input/output */
> +    if( v4l2_ioctl( i_fd, i_ioctl, index ) < 0 )
> +    {
> +        msg_Err( p_obj, "cannot set %s %u: %m",
> +                 (i_type == V4L2_CAP_VIDEO_CAPTURE) ? "input" : "output",
> +                 *index );
> +        return false;
> +    }
> +    msg_Dbg( p_obj, "%s set to %u",
> +             (i_type == V4L2_CAP_VIDEO_CAPTURE) ? "input" : "output",
> +             *index );
> +
> +    return true;
> +}

This function also creates useless clutter just to share an ioctl(). I don't 
see the point in factoring it across input and output. Same comment for the 
modulator code, which is output-specific:

> +/* List and set tuner/modulator caps */
> +bool v4l2_set_tuner_freq( vlc_object_t *p_obj, int i_type, int i_fd,
> +                          uint32_t idx, uint32_t freq, int32_t audmode )
(...)

> +bool v4l2_negociate_chroma( vlc_object_t *p_obj, int i_fd,
> +                            vlc_fourcc_t i_requested_fourcc,
> +                            struct v4l2_format *fmt,
> +                            struct v4l2_fmtdesc *p_codecs, uint32_t ncodec
> ) +{
> +    /* Test and set Chroma */
> +    fmt->fmt.pix.pixelformat = 0;
> +    if( i_requested_fourcc )
> +    {
> +        /* User specified chroma */
> +        get_v4l2pixelformat_by_fourcc( &fmt->fmt.pix.pixelformat,
> i_requested_fourcc ); +
> +        /* Try and set user chroma */
> +        bool b_error = !IsPixelFormatSupported( p_codecs, ncodec,
> +                                                fmt->fmt.pix.pixelformat
> ); +        if( !b_error && fmt->fmt.pix.pixelformat )
> +        {
> +            if( v4l2_ioctl( i_fd, VIDIOC_S_FMT, fmt ) < 0 )
> +            {
> +                fmt->fmt.pix.field = V4L2_FIELD_ANY;
> +                if( v4l2_ioctl( i_fd, VIDIOC_S_FMT, fmt ) < 0 )
> +                {
> +                    fmt->fmt.pix.field = V4L2_FIELD_NONE;
> +                    b_error = true;
> +                }
> +            }
> +        }
> +        if( b_error )
> +        {
> +            msg_Warn( p_obj, "requested chroma not supported. "
> +                      " Trying default." );
> +            fmt->fmt.pix.pixelformat = 0;
> +        }
> +    }

I know this is coming from the V4L2 input but I am rewriting this code because 
it is wrong. So it should definitely not be factored.

> +
> +    /* If no user specified chroma, find best */
> +    /* This also decides if MPEG encoder card or not */
> +    if( !fmt->fmt.pix.pixelformat )
> +    {
> +        unsigned int i;
> +        for( i = 0; i < ARRAY_SIZE( p_chroma_fallbacks ); i++ )
> +        {
> +            fmt->fmt.pix.pixelformat = p_chroma_fallbacks[i];
> +            if( IsPixelFormatSupported( p_codecs, ncodec,
> +                                        fmt->fmt.pix.pixelformat ) )
> +            {
> +                if( v4l2_ioctl( i_fd, VIDIOC_S_FMT, fmt ) >= 0 )
> +                    break;
> +                fmt->fmt.pix.field = V4L2_FIELD_ANY;
> +                if( v4l2_ioctl( i_fd, VIDIOC_S_FMT, fmt ) >= 0 )
> +                    break;
> +                fmt->fmt.pix.field = V4L2_FIELD_NONE;
> +            }
> +        }
> +
> +        if( i == ARRAY_SIZE( p_chroma_fallbacks ) )
> +        {
> +            msg_Warn( p_obj, "Could not select any of the default chromas;
> attempting to open as MPEG encoder card (access)" ); +            return
> false;
> +        }
> +    }
> +    return true;
> +}

-- 
Rémi Denis-Courmont
http://www.remlab.net/
http://fi.linkedin.com/in/remidenis



More information about the vlc-devel mailing list