[FFmpeg-devel] patch for libdc1394.c

[Alessandro Sappia]

Roman Shaposhnik ha scritto:
> On Jan 5, 2008, at 2:39 PM, Alessandro Sappia wrote:
>   
[cut]
>
>> +#include <sched.h>
>>     
>
>    Why is this include needed?
>
>   
good point, I used it to do some high def fps computation,
it's not needed
>>  typedef struct dc1394_data {
>> +#ifdef ENABLE_LIBDC1394_1
>>      raw1394handle_t handle;
>>      dc1394_cameracapture camera;
>> +#else
>> +    dc1394_t *d;
>> +    dc1394camera_t *camera;
>> +    dc1394video_frame_t * frame;
>> +#endif
>>      int current_frame;
>>      int fps;
>> -
>>     
>
>    There's lots of cosmetical changes. Get rid of them.
>
>   
ok


>> -    {     0, FRAMERATE_30    } /* default -- gotta be the last one */
>> +    { 30000, FRAMERATE_30    } /* default -- gotta be the last one */
>>     
>
>     I don't want you to change the logic here. I don't see any reason  
> for this.
>
>   
I changed the logic here because with either the old version, and the
new one
simply the current logic doesn't work for me. I took the logic from
libavdevice/v4l2.c

>    See above. I don't like #ifdef's.
>
>   
>>  static int dc1394_read_header(AVFormatContext *c,  
>> AVFormatParameters * ap)
>>  {
>>      dc1394_data* dc1394 = c->priv_data;
>>      AVStream* vst;
>> +#ifdef ENABLE_LIBDC1394_1
>>      nodeid_t* camera_nodes;
>> -    int res;
>> -    struct dc1394_frame_format *fmt;
>> -    struct dc1394_frame_rate *fps;
>> +#else
>> +    dc1394camera_list_t *list;
>> +    int width,height;
>> +#endif
>> +    int res, i;
>> +    struct dc1394_frame_format *fmt = dc1394_frame_formats;
>> +    struct dc1394_frame_rate *fps = dc1394_frame_rates;
>>
>> -    for (fmt = dc1394_frame_formats; fmt->width; fmt++)
>> -         if (fmt->pix_fmt == ap->pix_fmt && fmt->width == ap- 
>>     
>>> width && fmt->height == ap->height)
>>>       
>> +    for (i = 1 ; i < sizeof(dc1394_frame_formats)/sizeof(struct  
>> dc1394_frame_format); i++,fmt++) {
>> +         if (fmt->pix_fmt == ap->pix_fmt && fmt->width == ap- 
>>     
>>> width && fmt->height == ap->height){
>>>       
>>               break;
>> +         }
>> +    }
>>     
>
>    I see no reason for this change.
>
>   
well, launching with no fps or no video mode defined,
the current logic ends with a non working fps and video mode

the current logic is:
    for (fmt = dc1394_frame_formats; fmt->width; fmt++)
         if (fmt->pix_fmt == ap->pix_fmt && fmt->width == ap->width &&
fmt->height == ap->height)
               break;

as you may check, it no ap->pix_fmt,ap->width, ap->height, the choice of
this for
will be the last (default) one.
That's not a problem for libdc1394 because it will always get the right
default value, but
(lines 93 and following)?
    vst->codec->time_base.den = fps->frame_rate;
    vst->codec->time_base.num = 1000;
    vst->codec->width = fmt->width;
    vst->codec->height = fmt->height;
    vst->codec->pix_fmt = fmt->pix_fmt;

As you may see the above logic set a non working framerate den (0)
and wrong width, height, pix_fmt.

this end up in a
picture size invalid (0x0)
[libdc1394 @ 0x8417854]Can't prepare camera for the DMA capture

(output from the clean source tree)
>   
>> +    if (res != DC1394_SUCCESS || list == 0 ) {
>>     
>
>    either !list or list == NULL
>
>   
good point ^^
>> +        dc1394_camera_free_list (list);
>>     
>
>    are you sure dc1394_camera_free_list (garbage) will work?
>
>
>   
from libdc1394 header:
void dc1394_camera_free_list(dc1394camera_list_t *list);

>> +        goto out;
>> +    }
>>
>> +    if (list->num < 1) {
>> +        av_log(c, AV_LOG_ERROR, "no cameras found :(\n");
>> +        goto out_handle;
>> +    }
>> +    dc1394->camera = dc1394_camera_new (dc1394->d, list->ids 
>> [0].guid);
>> +    if (list->num > 1) {
>> +        av_log(c, AV_LOG_INFO, "Working with the first camera found 
>> \n");
>> +    }
>> +    /* Freeing list of cameras */
>> +    dc1394_camera_free_list (list);
>> +
>> +    /* Select MAX Speed possible from the cam */
>> +    if (dc1394->camera->bmode_capable>0) {
>> +       dc1394_video_set_operation_mode(dc1394->camera,  
>> DC1394_OPERATION_MODE_1394B);
>> +       i = DC1394_ISO_SPEED_800;
>> +    } else {
>> +       i = DC1394_ISO_SPEED_400;
>> +    }
>> +
>> +    for (res = DC1394_FAILURE; i >= DC1394_ISO_SPEED_MIN && res !=  
>> DC1394_SUCCESS; i--) {
>> +            res=dc1394_video_set_iso_speed(dc1394->camera, i);
>> +    }
>> +    if (res != DC1394_SUCCESS) {
>> +        av_log(c, AV_LOG_ERROR, "Couldn't set ISO Speed\n");
>> +        goto out_handle;
>> +    }
>> +
>> +    if (dc1394_video_set_mode(dc1394->camera, fmt->frame_size_id) ! 
>> = DC1394_SUCCESS) {
>> +        av_log(c, AV_LOG_ERROR, "Couldn't set video format\n");
>> +        goto out_handle;
>> +    }
>> +
>> +    if (dc1394_video_set_framerate(dc1394->camera,fps- 
>>     
>>> frame_rate_id) != DC1394_SUCCESS) {
>>>       
>> +        av_log(c, AV_LOG_ERROR, "Couldn't set framerate %d \n",fps- 
>>     
>>> frame_rate);
>>>       
>> +        goto out_handle;
>> +    }
>> +    if (dc1394_capture_setup(dc1394->camera, 10,  
>> DC1394_CAPTURE_FLAGS_DEFAULT)!=DC1394_SUCCESS) {
>> +        av_log(c, AV_LOG_ERROR, "Cannot setup camera \n");
>> +        goto out_handle;
>> +    }
>> +
>> +    if (dc1394_video_set_transmission(dc1394->camera, DC1394_ON) ! 
>> =DC1394_SUCCESS) {
>> +        av_log(c, AV_LOG_ERROR, "Cannot start capture\n");
>> +        goto out_handle_dma;
>> +    }
>> +#endif
>>      return 0;
>>
>>  out_handle_dma:
>> +#ifdef ENABLE_LIBDC1394_1
>>      dc1394_dma_unlisten(dc1394->handle, &dc1394->camera);
>>      dc1394_dma_release_camera(dc1394->handle, &dc1394->camera);
>> +#else
>> +    dc1394_capture_stop(dc1394->camera);
>> +    dc1394_video_set_transmission(dc1394->camera, DC1394_OFF);
>> +#endif
>>  out_handle:
>> +#ifdef ENABLE_LIBDC1394_1
>>      dc1394_destroy_handle(dc1394->handle);
>> +    av_free(dc1394->camera);
>> +#else
>> +    dc1394_camera_free (dc1394->camera);
>> +#endif
>>  out:
>> +#ifndef ENABLE_LIBDC1394_1
>> +    dc1394_free(dc1394->d);
>> +#endif
>>      return -1;
>>  }
>>     
>
>    You know what, #ifdefs suck. Please refactor the code in two  
> separate functions.
>
>   
>>      /* discard stale frame */
>>      if (dc1394->current_frame++) {
>> +#ifdef ENABLE_LIBDC1394_1
>>          if (dc1394_dma_done_with_buffer(&dc1394->camera) !=  
>> DC1394_SUCCESS)
>> +#else
>> +        if (dc1394_capture_enqueue(dc1394->camera, dc1394->frame) ! 
>> = DC1394_SUCCESS)
>> +#endif
>>     
>
>    This strikes me as wrong.
>
>   
why wrong ?
the dc1394_capture_enqueue function works by putting back in the
DMA ring buffer the frame pointed by the dc1394->frame; that's pointing
to the old frame, and I did not touch the logic, here I just changed
the function name between the old and the new versions.


Thanks to all for the suggestion

Alessandro