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

[Roman Shaposhnik]

Hello,

there's been quite some emailing going back and forth. I'll try
to reply in the LIFO order, but remind me (in private) if I
miss anything.

On Mon, 2008-01-07 at 03:33 +0100, Alessandro Sappia wrote:
> Hi again,
> I read carefully all your suggestions, and I took them before resubmitting
> this patch.

  Great.

> - fixed configure selection of the right version library

  This seems to be clean. M?ns, I presume it'll be ok for me
to apply this portion in a single transaction with the other one?

> - removed all cosmetic stuff
> - removed unrelated changes
> - removed personal testing header (ops... why I left it here before?)

   Good.

> - changed #ifdef to #if
> - changed a check to be type consistent
> - refactored the code to be easier readable

   Ok, I do have one last question here, though. It is 
minor nit, but if anybody besides me cares we might as well
take care of it. So, here's the deal: what you currently have
in your patch is fine and readable, but personally I really
dislike #if[def] sections which are longer than just a couple
of lines. On top of that, one would hope that support for 
libdc1394 v. 1 is going to be dropped sometime in the future.
Hence, I'd rather have this (in skeletal form):

   #if ENABLE_LIBDC1394_2
   #include <dc1394/dc1394.h>
   #elif ENABLE_LIBDC1394_1
   #include <libraw1394/raw1394.h>
   #include <libdc1394/dc1394_control.h>
   #define DC1394_VIDEO_MODE_320x240_YUV422 MODE_320x240_YUV42
   ....
   #define DC1394_FRAME_RATE_1_875 FRAME_RATE_1_875
   ....
   #endif /* ENABLE_LIBDC1394_1 */
   
   static inline int dc_1394_read_header_common(...)

   static int dc1394_v1_read_header(...)
   {
       dc_1394_read_header_common();
       ....
   }

   static int dc1394_v2_read_header(...)
   {
       dc_1394_read_header_common();
       ....
   }

   .....
  
   #if ENABLE_LIBDC1394_1
   AVInputFormat libdc1394_demuxer = {
    .name           = "libdc1394",
    .long_name      = "dc1394 v1 A/V grab",
    .priv_data_size = sizeof(struct dc1394_data),
    .read_header    = dc1394_v1_read_header,
    .read_packet    = dc1394_v1_read_packet,
    .read_close     = dc1394_v1_close,
    .flags          = AVFMT_NOFILE
   };
   #elif ENABLE_LIBDC1394_2
   AVInputFormat libdc1394_demuxer = {
    .name           = "libdc1394",
    .long_name      = "dc1394 v2 A/V grab",
    .priv_data_size = sizeof(struct dc1394_data),
    .read_header    = dc1394_v2_read_header,
    .read_packet    = dc1394_v2_read_packet,
    .read_close     = dc1394_v2_close,
    .flags          = AVFMT_NOFILE
   }; 
   #endif

> >> +        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.

 Cool. Thanks for the explanation. 
 
> 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().

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

  Well, beyond the question above and somebody for whom English is a 
mother tongue looking over the output messages, I believe this patch
is now ok to be included. Good work!

Thanks,
Roman.