[MPlayer-dev-eng] [PATCH] new libvo driver: vo_ivtv [ask for review]

[Reimar Döffinger]

Hello,
On Tue, Jul 11, 2006 at 09:24:36PM +0200, Benjamin Zores wrote:
> +  {"device",   OPT_ARG_MSTRZ,     &device,       NULL},

This returns a string in buffer allocated via malloc in the parser.

> +  while (ioctl (ivtv_fd, IVTV_IOC_S_STOP_DECODE, &sd) < 0)
> +  {
> +    if (errno != EBUSY)
> +    {
> +      mp_msg (MSGT_VO, MSGL_ERR,
> +              "IVTV_IOC_STOP_DECODE: %s\n", strerror (errno));
> +      return 1;
> +    }
> +  }

Hmm... we might potentially hang here forever, maybe it would be a good
idea to tell the user that the device is in use and we're waiting for it
to become free?

> +  while (ioctl (ivtv_fd, IVTV_IOC_S_START_DECODE, &sd1) < 0)

Same here of course.

> +  device = NULL;

you can do
device = strdup (DEFAULT_MPEG_DECODER);
here instead of the code below.

> +  dev_name = device ? strdup (device) : strdup (DEFAULT_MPEG_DECODER);

Do not use strdup on device! it is already malloc memory, like this you
will have a leak.
Combined with the suggestion above, you also don't really need the
both a "device" and a "dev_name" variable.

> +  ivtv_fd =3D open (dev_name, O_RDWR);
> +  if (ivtv_fd < 0) 
> +  { =20
> +    free (dev_name);

Hmm... dev_name is only used once more, namely here:

> +  /* display device name */
> +  mp_msg (MSGT_VO, MSGL_INFO, "VO: [ivtv] using %s\n", dev_name);

Wouldn't it be simpler to just move this message e.g. directly before
the open and free dev_name right after the open? Then you could get rid
of most free()s. Also it might be a good idea to show the user the
device name you try even when it fails (I'd actually say especially).
Or actually, if you continue to use the global "device" variable you
could just free it on uninit, the you can also use it in messages during
playback should you ever want to. Would also allow implementing stuff
like "free device on pause".

> +    mp_msg (MSGT_VO, MSGL_ERR, "vo_ivtv: %s\n", strerror (errno));

You have [ivtv] everywhere else. Also something like "error opening"
might be a good idea. And since initialization will fail when this
message appears, I think the right message level would be MSGL_FATAL.

> +  vout.index = 0;
> +  err = 1;
> +  mp_msg (MSGT_VO, MSGL_INFO, "VO: [ivtv] Available video outputs: ");
> +  while (ioctl (ivtv_fd, VIDIOC_ENUMOUTPUT, &vout) >= 0)
> +  {
> +    err = 0;
> +    mp_msg (MSGT_VO, MSGL_INFO, "'#%d, %s' ", vout.index, vout.name);
> +    vout.index++;
> +  }
> +  if (err)
> +  {
> +    mp_msg (MSGT_VO, MSGL_INFO, "none\n");
> +    free (dev_name);
> +    return -1;
> +  }
> +  else
> +    mp_msg (MSGT_VO, MSGL_INFO, "\n");

This seems rather confusing to me, both code-wise and from what the user
seems, also since the err case should be MSGL_FATAL.
I don't know a really good alternative though.

> +static void
> +flip_page (void)
> +{
> +  if (ivtv_fd < 0)
> +    return;
> +  
> +  if (pes->size > 0)
> +  {
> +    int wrote, curlen;
> +    char *buf_ptr;
> +    
> +    buf_ptr = pes->data;
> +    curlen = pes->size;
> +          
> +    while (curlen > 0)
> +    {
> +      wrote = write (ivtv_fd, buf_ptr, curlen);
> +      buf_ptr = buf_ptr + wrote;
> +      curlen -= wrote;
> +    }

I'd also check for pes == NULL and either set pes->size to 0 or pes to
NULL after writing the data, just in case flip_page gets called twice...

Greetings,
Reimar Döffinger