[FFmpeg-devel] patch for libdc1394.c
Roman Shaposhnik
rvs
Sun Jan 6 00:53:42 CET 2008
On Jan 5, 2008, at 2:39 PM, Alessandro Sappia wrote:
> one is for configure:
> it checks for the existence of both the old and the new libdc1394
M?ns et al. need to take care of reviewing it.
> the second is for libavdevice/libdc1394.c
> a define chooses which version of the code should be used.
>
> +#include "config.h"
> +#ifdef ENABLE_LIBDC1394_1
> #include <libraw1394/raw1394.h>
> #include <libdc1394/dc1394_control.h>
> +#else
> +#include <sched.h>
Why is this include 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.
>
> - { 0, 0, 0, MODE_320x240_YUV422 } /* default -- gotta be
> the last one */
> + { 320, 240, PIX_FMT_UYVY422, MODE_320x240_YUV422 } /* default
> -- gotta be the last one */
I don't want you to change the logic here. I don't see any reason
for this.
> +#else
> + { 320, 240, PIX_FMT_UYVY422, DC1394_VIDEO_MODE_320x240_YUV422 },
> + { 640, 480, PIX_FMT_UYYVYY411,
> DC1394_VIDEO_MODE_640x480_YUV411 },
> + { 640, 480, PIX_FMT_UYVY422, DC1394_VIDEO_MODE_640x480_YUV422 },
> + { 320, 240, PIX_FMT_UYVY422,
> DC1394_VIDEO_MODE_320x240_YUV422 } /* default -- gotta be the last
> one */
> +#endif
> };
The DC1394_VIDEO_MODE_320x240_YUV422 vs MODE_320x240_YUV422 should be
handled through the macro substitution.
> - { 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.
> +#else
> + { 1875, DC1394_FRAMERATE_1_875 },
> + { 3750, DC1394_FRAMERATE_3_75 },
> + { 7500, DC1394_FRAMERATE_7_5 },
> + { 15000, DC1394_FRAMERATE_15 },
> + { 30000, DC1394_FRAMERATE_30 },
> + { 60000, DC1394_FRAMERATE_60 },
> + {120000, DC1394_FRAMERATE_120, },
> + {240000, DC1394_FRAMERATE_240 },
> + { 30000, DC1394_FRAMERATE_30 } /* default -- gotta be the
> last one */
> +#endif
> };
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.
> - for (fps = dc1394_frame_rates; fps->frame_rate; fps++)
> - if (fps->frame_rate == av_rescale(1000, ap-
> >time_base.den, ap->time_base.num))
> + for (i = 1 ; i < sizeof(dc1394_frame_rates)/sizeof(struct
> dc1394_frame_rate); i++,fps++) {
> + if (fps->frame_rate == av_rescale(1000, ap-
> >time_base.den, ap->time_base.num)) {
> break;
> + }
> + }
I see no reason for this change.
> /* create a video stream */
> vst = av_new_stream(c, 0);
> @@ -102,6 +141,7 @@
> vst->codec->bit_rate = av_rescale(dc1394->packet.size * 8, fps-
> >frame_rate, 1000);
>
> /* Now lets prep the hardware */
> +#ifdef ENABLE_LIBDC1394_1
> dc1394->handle = dc1394_create_handle(0); /* FIXME: gotta have
> ap->port */
> if (!dc1394->handle) {
> av_log(c, AV_LOG_ERROR, "Can't acquire dc1394 handle on
> port %d\n", 0 /* ap->port */);
> @@ -131,15 +171,87 @@
> av_log(c, AV_LOG_ERROR, "Can't start isochronous
> transmission\n");
> goto out_handle_dma;
> }
> +#else
> + /* Now lets prep the hardware */
> + dc1394->d = dc1394_new();
> + res = dc1394_camera_enumerate (dc1394->d, &list); /* FIXME:
> gotta have ap->port */
> + if (res != DC1394_SUCCESS || list == 0 ) {
either !list or list == NULL
> + av_log(c, AV_LOG_ERROR, "Unable to look for an IIDC camera
> \n\n"
> + "On Linux, please check that\n"
> + " - the kernel modules
> `ieee1394',`raw1394' and `ohci1394' are loaded \n"
> + " - you have read/write access
> to /dev/raw1394\n"
> + " - you have an IIDC camera
> connected\n\n");
> + dc1394_camera_free_list (list);
are you sure dc1394_camera_free_list (garbage) will work?
> + 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.
> +#else
> + res = dc1394_capture_dequeue(dc1394->camera,
> DC1394_CAPTURE_POLICY_WAIT, &dc1394->frame);
> +#endif
> if (res == DC1394_SUCCESS) {
> +#ifdef ENABLE_LIBDC1394_1
> dc1394->packet.data = (uint8_t *)(dc1394-
> >camera.capture_buffer);
> - dc1394->packet.pts = (dc1394->current_frame * 1000000) /
> dc1394->fps;
> - res = dc1394->packet.size;
> +#else
> + dc1394->packet.data = (uint8_t *)(dc1394->frame->image);
> +#endif
> + dc1394->packet.pts = (dc1394->current_frame * 1000000) /
> (dc1394->fps);
> + res = dc1394->packet.size = dc1394->frame->image_bytes;
> } else {
> av_log(c, AV_LOG_ERROR, "DMA capture failed\n");
> dc1394->packet.data = NULL;
> + dc1394->packet.size = 0;
> res = -1;
> }
> -
> *pkt = dc1394->packet;
> return res;
> }
> @@ -173,12 +294,18 @@
> static int dc1394_close(AVFormatContext * context)
> {
> struct dc1394_data *dc1394 = context->priv_data;
> -
> +#ifdef ENABLE_LIBDC1394_1
> dc1394_stop_iso_transmission(dc1394->handle, dc1394-
> >camera.node);
> dc1394_dma_unlisten(dc1394->handle, &dc1394->camera);
> dc1394_dma_release_camera(dc1394->handle, &dc1394->camera);
> dc1394_destroy_handle(dc1394->handle);
> -
> + av_free(dc1394->camera);
> +#else
> + dc1394_video_set_transmission(dc1394->camera, DC1394_OFF);
> + dc1394_capture_stop(dc1394->camera);
> + dc1394_camera_free(dc1394->camera);
> + dc1394_free(dc1394->d);
> +#endif
> return 0;
> }
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> http://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
More information about the ffmpeg-devel
mailing list