[FFmpeg-devel] PATCH grab.c, adding support for hardware based VIDIOSFPS

[Benoit Fouet]

Hi,

mmh wrote:
> Benoit Fouet writes:
>  > mmh wrote:
>  > > Benoit Fouet writes:
>  > >  > Hi,
>  > >  > 
>  > >  > mmh wrote:
>  > >
>  > > I don't think we need to change my original patch.
>  > >
>  > > the drivers default behavior is to return an error ie negative value
>  > > if its not supported in the switch (cmd) { case blah1: .... in the video 4
>  > > linux stack.
>  > >   
>  > 
>  > this is wrong.
>  > from my videodev2.h:
>  > #define VIDIOC_S_RDS            _IOWR ('V', BASE_VIDIOC_PRIVATE+11,
>  > struct v4l2_radio_rds_set)
>  > 
>  > i can (if i want) change the BASE_VIDIOC_PRIVATE+11 by
>  > BASE_VIDIOC_PRIVATE+20
>  > this ioctl will be defined, but will not do what you want...
>  > 
>  > moreover, it can just return without any error, and lead you to believe
>  > that framerate is accepted by the camera, even though this is not true...
>
> Ben,
>
> I see it, the thing can have various meanings based on platform, So I
> guess if I remove this from the grab.c implementation.
>
> #ifndef VIDIOSFPS
> #define VIDIOSFPS		_IOW('v',BASE_VIDIOCPRIVATE+20, int)			/* Set fps */
> #endif
>
>
>   

yes, i really think this should be removed
and, FWIW, syntax in v4l2 is, most of the time, as follows:
VIDIOC_S_*
i don't know if it's planned to add such a feature in future videodev
version, though...

> And then if the VIDIOSFPS is defined by the system v4l includes it
> would be acceptable to do this?
>
>     fps = s->frame_rate / s->frame_rate_base;
>     s->sw_fps_reduction  = 1;
> #ifdef VIDIOSFPS
>     s->sw_fps_reduction  = ioctl( video_fd, VIDIOSFPS, &fps ) < 0;
> #endif
>
>
>   

as i already mentionned, i'd prefer to put the fps variable definition
under ifdef

#ifdef V4L_FLAG
    int fps;
#endif

[...]

#ifdef V4L_FLAG
    fps = s->frame_rate / s->frame_rate_base;
    s->sw_fps_reduction = ioctl( video_fd, VIDIOSFPS, &fps ) < 0;
#else
    s->sw_fps_reduction = 1;
#endif

> Thanks for struggling through this issue with me.
>
>   

you're welcome :)

Ben
-- 
Purple Labs S.A.
www.purplelabs.com