[FFmpeg-devel] [PATCH] new version of libdc1394 - resubmitting after review

[Alessandro Sappia]

Hi again,
I read carefully all your suggestions, and I took them before resubmitting
this patch.

These are the results:

- fixed configure selection of the right version library
- removed all cosmetic stuff
- removed unrelated changes
- removed personal testing header (ops... why I left it here before?)
- changed #ifdef to #if
- changed a check to be type consistent
- refactored the code to be easier readable

I think You all may need a more comprehensive support for the following
complain.

Roman Shaposhnik wrote:

>    The DC1394_VIDEO_MODE_320x240_YUV422 vs MODE_320x240_YUV422 should be
> handled through the macro substitution.

The choice of not handling these macro that way is based on the choice of
libdc1394 developers, when they decided to apply a DC1394_ prefix to
avoid namespace pollution.
I think that keeping these defines this way let the reader easily understand
that DC1394_* stuff is non ffmpeg internal, while it comes from outside
headers. IMHO, they should be as they are. Sorry.

>    are you sure dc1394_camera_free_list (garbage) will work?
There is no way in that line where the list pointer could become garbage:
the pointer is init'd by the dc1394_camera_enumerate some lines above,
with a calloc (which returns NULL if it fails); free(NULL) means "no
operation
is performed". In any case, I refactored that part to be simpler,
avoiding also
a free(NULL).

>    You know what, #ifdefs suck. Please refactor the code in two  
> separate functions.
Refactored and using #if instead of #ifdef. Now the old way and the new one
have nothing but the name in common.

>> +        if (dc1394_capture_enqueue(dc1394->camera, dc1394->frame) != DC1394_SUCCESS)
>
>    This strikes me as wrong.
>
>   
This put back the pointer to the current frame into the dma ring buffer, so
a future dc1394_capture_dequeue() could use it again. The form is different,
but the underline code is almost the same as the
dc1394_dma_done_with_buffer()
used in the old version. The difference is that the new version has
exposed the video
frame structure directly, while the previous one had the frame inside
the camera
structure.

By the way, (and unrelated to this patch):
if frame rate is not set to the appropriate value while launching ffmpeg
(aka using ffplay)
you get a division by zero while updating dc1394->packet.pts inside
dc1394_read_packet().
I submitted a patch that solves this unrelated issue yesterday.
(Message-ID: <47802ABA.7060302 at ngi.it>)

I hope that the attached patch will be good for you all.

In the meanwhile, Kind regards


Alessandro


-------------- next part --------------
A non-text attachment was scrubbed...
Name: use_libdc1394_version_1_or_version_2.patch
Type: text/x-patch
Size: 9576 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080107/64cd68de/attachment.bin>