GNOME Bugzilla – Bug 165193
Patch for ov51x v4ljpegsrc
Last modified: 2005-02-03 22:10:06 UTC
Attaching a patch that adds a v4ljpegsrc derived from v4lsrc. I had to do this as a separate element because V4L has no format for Jpeg output frames, so the driver simply returns JPEG data the the framebuffer when you set up for RGB frames. Examples of ov51x camera are the Sony EyeToys. Putting the patch here because I think it's a bit dirty - anyone else got a better way to implement it? At the moment it derives from v4lsrc and replaces the link and get functions on the pads behind the parent class' back.
Created attachment 36508 [details] [review] Patch for v4ljpegsrc
I only see atches to existing files, can you attach the new files, too?
Created attachment 36535 [details] new gstv4ljpegsrc.[ch] files silly me, here's the new files to go along with the patch
static gfloat gst_v4lsrc_get_fps (GstV4lSrc * v4lsrc); appears a copy from the v4lsrc version, just expose it, don't copy. _link: don't throw a GST_ELEMENT_ERROR() on caps negotiation failure, because it is not fatal. It is fatal when no caps was negotiated after all caps nego is over, which can be multiple runs. This may me a bug in v4lsrc as well, so fix it there too. ;). I'm not very sure whether using _try_capture in _link is a good idea, because it takes rather long, but ohwell... We'll think of optimization later. fourcc = GST_MAKE_FOURCC ('R', 'G', 'B', ' '); ??? That's five-year old API. :). It seems unused, too, so remove it. _get: using 2 bytes of shared hardware memory for the size of the data? Whoever thought of this should be burned alive, POSIX has explicit return values for that. Not your fault, but you get the point. ;). For timestamp copying, use gst_buffer_stamp(). Also: unsigned short *read_size = ((unsigned short *)(GST_BUFFER_DATA (buf))); jpeg_size = (int)(read_size[0]) * 8; I don't know what you're trying to do here, but you should probably use GST_READ_UINT16_LE(). This is non-portable. Check that the given size is inside the buffer, else error out, just for safety (people could try this on their TV card, which would give interesting segfaults). General: I think it's OK. It may not be the nicest API I've seen around, but the code in the element is nice 'n simple.
Additional note: do these cams have an extra flag set? It'd be nice to test this in the _open function in v4l_calls.c, so that we don't open the device as a wrong type. Also, this'd make device probing in Cupid continue to work.
Thanks for the review. Regarding gst_v4lsrc_try_capture in the link function, that comes from v4lsrc so if there's a better way to do it, there's 2 spots to improve. Unfortunately, I can't see anything we can use to distinguish the cameras from others, other than the string in the driver name, and even that doesn't help because the same driver also does ov511/ov518 cameras that produce uncompressed data. I've written to the driver author to ask him, but I think basically it is just required that the user knows their using one of these cameras and uses a different element, otherwise they'll just get pretty flickering static for output.
Hm, crap. Ohwell. Anyway, you applied it, so let's close this one.