IIDC Camera Control Library

On Thu, 2003-11-20 at 11:59, Johann Schoonees wrote:
> dedicated function for polling.  Here it is anyway in case you are 
> desperate.  If at all possible we should wait for the next release of 
> libdc1394 which I hope will fix some of these concerns.
> 
> I am not going to have time to send in a patch to extend 
> dc1394_cameracapture etc. because of hectic project pressure.  If any 
> of the developers would like to pick up some of the ideas below that 
> would be great.

Thank you Johann. Unfortunately, due to projects myself, it may take me
some time to address the DMA capture flaws you pointed out, perhaps as
late as February 2004. Also, unfortunately, libdc1394 is not a rather
important project for me. Heck, I just use it for playing around with my
cheap litte webcam :-). IOW, other developers, for whom this project may
be more critical, are welcome to be more involved.

Dan Dennedy wrote:
> On Thu, 2003-11-20 at 11:59, Johann Schoonees wrote:
> 
>>dedicated function for polling.  Here it is anyway in case you are 
>>desperate.  If at all possible we should wait for the next release of 
>>libdc1394 which I hope will fix some of these concerns.
>>
>>I am not going to have time to send in a patch to extend 
>>dc1394_cameracapture etc. because of hectic project pressure.  If any 
>>of the developers would like to pick up some of the ideas below that 
>>would be great.
> 
> 
> Thank you Johann. Unfortunately, due to projects myself, it may take me
> some time to address the DMA capture flaws you pointed out, perhaps as
> late as February 2004. Also, unfortunately, libdc1394 is not a rather
> important project for me. Heck, I just use it for playing around with my
> cheap litte webcam :-). IOW, other developers, for whom this project may
> be more critical, are welcome to be more involved.
> 
> 
> 

Hi Dan

OK, I can feel you twisting my arm.  I need to get an urgent demo out 
of the way first and then I should be able to submit a patch in about 
4 weeks from now.

I assume that anyone else who would like to contribute are welcome 
too.  I am new to this list so I am not sure who calls the shots.

I propose to do the following:

  1. Not change the way drop_frames is used in 
dc1394_dma_multi_capture() at present, with one exception (see 2 below).

  2. Change the filltime returned when drop_frames!=0 so that it 
corresponds to the frame returned in the capture_buffer pointer. 
(Currently the filltime refers to the first dropped frame if any.)

  3. Add a new struct timeval pointer member to the 
dc1394_cameracapture stucture called something like 
dma_extra_filltimes which points to an array of filltimes of frames in 
dma_extra_buffer.  (Curently filltimes of all frames in 
dma_extra_buffer are lost.)

  4. Add a new flag member to the dc1394_cameracapture stucture called 
something like dma_extra_buffer_enable which switches copying to 
dma_extra_buffer on or off.  (We will have to decide what to do if 
both drop_frames and dma_extra_buffer_enable flags are set.)

  5. Added dma_extra_buffer_enable to the argument list of 
dc1394_dma_setup_capture() and initialize it in the 
dc1394_cameracapture stucture.

  6. Allocate memory for dma_extra_buffer in 
dc1394_dma_multi_capture() if 
dma_extra_buffer_enable&&dma_extra_buffer==NULL.  That enables the 
user to request extra buffers by modifying dc1394_cameracapture on the 
fly.  (Currently this could cause a segfault.)

  7. Submit a new local function called something like 
_dc1394_alloc_extra_buffer() which allocates memory for extra frames 
and filltimes, and call it from dc1394_dma_setup_capture().

  8. Free dma_extra_filltimes in dc1394_release_camera().

  9. Let dma_extra_buffer_enable (not !drop_frames) cause memory to be 
allocated for dma_extra_buffer in dc1394_dma_setup_capture().

10. Add a new int member to the dc1394_cameracapture structure called 
something like dma_buffers_behind which either returns the number of 
buffers by which we are behind in the DMA ring buffer (if 
!drop_frames) or returns the number of buffers dropped (if 
drop_frames).  (Currently the user can't find out these things.)

11. Submit a new function called dc1394_dma_multi_capture_poll() which 
  is identical to dc1394_dma_multi_capture() except that, if a frame 
is not available, it returns immediately with DC1394_SUCCESS and 
capture_buffer==NULL.  (Probably best to move the code that is common 
to both dc1394_dma_multi_capture() and dc1394_dma_multi_capture_poll() 
into a shared local function.)

Gosh, that list just carried on growing.  Better stop before I promise 
too much. :-)

I think these changes will maintain reasonable compatibility with 
existing code.  The only change for existing users should be to 
initialize dma_extra_buffer_enable=1 if they currently have 
drop_frames==0.

It also adds some needed features for people who need to know if 
frames were dropped and what the exact filltimes were of each frame.

Any comments?

Johann
-- 
^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^
Johann Schoonees                         Imaging & Sensing Team
Industrial Research Limited, PO Box 2225, Auckland, New Zealand
Phone +64 9 9203679 Fax +64 9 3028106 http://www.is.irl.cri.nz/

Johann,

This looks good to me. One thing to remember: format7 capture functions
will have to be updated too.

[snip]

>   4. Add a new flag member to the dc1394_cameracapture stucture called=20
> something like dma_extra_buffer_enable which switches copying to=20
> dma_extra_buffer on or off.  (We will have to decide what to do if=20
> both drop_frames and dma_extra_buffer_enable flags are set.)

I don't understand what will have to be decided. Please give me more
details.

>   5. Added dma_extra_buffer_enable to the argument list of=20
> dc1394_dma_setup_capture() and initialize it in the=20
> dc1394_cameracapture stucture.
>=20
>   6. Allocate memory for dma_extra_buffer in=20
> dc1394_dma_multi_capture() if=20
> dma_extra_buffer_enable&&dma_extra_buffer=3D=3DNULL.  That enables the=20
> user to request extra buffers by modifying dc1394_cameracapture on the=20
> fly.  (Currently this could cause a segfault.)

So in short this would let the user re-allocated the ring buffer on the
fly(to change its size)? Would this done by re-calling
dc1394_dma_setup_capture?

>   7. Submit a new local function called something like=20
> _dc1394_alloc_extra_buffer() which allocates memory for extra frames=20
> and filltimes, and call it from dc1394_dma_setup_capture().
>=20
>   8. Free dma_extra_filltimes in dc1394_release_camera().
>=20
>   9. Let dma_extra_buffer_enable (not !drop_frames) cause memory to be=20
> allocated for dma_extra_buffer in dc1394_dma_setup_capture().
>=20
> 10. Add a new int member to the dc1394_cameracapture structure called=20
> something like dma_buffers_behind which either returns the number of=20
> buffers by which we are behind in the DMA ring buffer (if=20
> !drop_frames) or returns the number of buffers dropped (if=20
> drop_frames).  (Currently the user can't find out these things.)

Counting dropped frames is a very neat feature. :-)

> 11. Submit a new function called dc1394_dma_multi_capture_poll() which=20
>   is identical to dc1394_dma_multi_capture() except that, if a frame=20
> is not available, it returns immediately with DC1394_SUCCESS and=20
> capture_buffer=3D=3DNULL.  (Probably best to move the code that is common=
=20
> to both dc1394_dma_multi_capture() and dc1394_dma_multi_capture_poll()=20
> into a shared local function.)

Yes, one code would be better. It will also be used by F7 capture
functions so we should re-use a maximum here.

Good ideas! ;-)

Damien

--=20
  _    Damien Douxchamps
 (=B0-   PhD Student / Research Assistant
 //\   Image Processing Group, Telecom Laboratory, UCL, Belgium
 V_/_  http://www.tele.ucl.ac.be/MEMBERS/Douxchamps_Damien_e.html

Damien Douxchamps wrote:
> Johann,
> 
> This looks good to me. One thing to remember: format7 capture functions
> will have to be updated too.

Yes, thanks for reminding me.

>>  4. Add a new flag member to the dc1394_cameracapture stucture called 
>>something like dma_extra_buffer_enable which switches copying to 
>>dma_extra_buffer on or off.  (We will have to decide what to do if 
>>both drop_frames and dma_extra_buffer_enable flags are set.)
> 
> 
> I don't understand what will have to be decided. Please give me more
> details.

dma_extra_buffer_enable!=0 means don't drop frames if possible but 
buffer those by which we are behind.

drop_frames!=0 means drop all frames by which we are behind.

It is possible that the user can set both flags.  Then do we drop 
frames or not?  We will have to decide which flag has precedence if 
both are set simultaneously.  My inclination is to play it safe and do 
the buffering, i.e. dma_extra_buffer_enable has precedence.

This is only a preliminary idea for the dma_extra_buffer_enable flag. 
  If anyone can think of a different flag that avoids the conflict 
(and breaks a minimum of existing code), please speak up.

>>  6. Allocate memory for dma_extra_buffer in 
>>dc1394_dma_multi_capture() if 
>>dma_extra_buffer_enable&&dma_extra_buffer==NULL.  That enables the 
>>user to request extra buffers by modifying dc1394_cameracapture on the 
>>fly.  (Currently this could cause a segfault.)
> 
> 
> So in short this would let the user re-allocated the ring buffer on the
> fly(to change its size)? Would this done by re-calling
> dc1394_dma_setup_capture?

No, I don't think that would work.  What I meant was that a user may 
change their mind after initially setting up with 
dma_extra_buffer_enable=0 (so that no memory gets allocated to 
dma_extra_buffer which is set to a null pointer).  If 
dma_extra_buffer_enable is set some time after initialization/setup 
then dc1394_dma_multi_capture() needs to detect this and quickly 
allocate some memory to dma_extra_buffer (preferably by calling the 
new proposed fundtion _dc1394_alloc_extra_buffer()) before 
memcpy(dma_extra_buffer,...) tries to access the null pointer.

> Good ideas! ;-)

Thanks for looking through the list.  By the time we get to code it we 
should have a clear idea of what is needed.

Regards,
Johann

-- 
^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^
Johann Schoonees                         Imaging & Sensing Team
Industrial Research Limited, PO Box 2225, Auckland, New Zealand
Phone +64 9 9203679 Fax +64 9 3028106 http://www.is.irl.cri.nz/

Hi Johann,

On Sun, 2003-11-23 at 22:44, Johann Schoonees wrote:
> >>  4. Add a new flag member to the dc1394_cameracapture stucture called=20
> >>something like dma_extra_buffer_enable which switches copying to=20
> >>dma_extra_buffer on or off.  (We will have to decide what to do if=20
> >>both drop_frames and dma_extra_buffer_enable flags are set.)
> >=20
> > I don't understand what will have to be decided. Please give me more
> > details.
>=20
> dma_extra_buffer_enable!=3D0 means don't drop frames if possible but=20
> buffer those by which we are behind.
>
> drop_frames!=3D0 means drop all frames by which we are behind.
>=20
> It is possible that the user can set both flags.  Then do we drop=20
> frames or not?  We will have to decide which flag has precedence if=20
> both are set simultaneously.  My inclination is to play it safe and do=20
> the buffering, i.e. dma_extra_buffer_enable has precedence.
>=20
> This is only a preliminary idea for the dma_extra_buffer_enable flag.=20
>   If anyone can think of a different flag that avoids the conflict=20
> (and breaks a minimum of existing code), please speak up.

I realize I've never quite understood the purpose of the extra_buffer!
;-) A quick reading of dc1394_capture didn't help but I still have two
comments:

- is it really interesting to have a multi_capture function? Wouldn't a
single capture function be enough? The comments in dc1394_capture (line
325) seem to indicate that the extra_buffer is only for multi capture,
so removing this feature might remove the problem. I personally think
that it's very easy to use a "for (each camera on the bus) do capture
end" so I don't really see the point of this multi capture at all. Then
again, I did not understand all the code ;)

- if the parameter is meaningful, then I would suggest to use some enum
type to limit the possibilities to the 3 meaningful ones: no drop/no
extra buffer, drop/no extra buffer and no drop/extra buffer. It could
still be a bit mask but it would be on 2 bits.

> >>  6. Allocate memory for dma_extra_buffer in=20
> >>dc1394_dma_multi_capture() if=20
> >>dma_extra_buffer_enable&&dma_extra_buffer=3D=3DNULL.  That enables the=20
> >>user to request extra buffers by modifying dc1394_cameracapture on the=20
> >>fly.  (Currently this could cause a segfault.)
> >=20
> >=20
> > So in short this would let the user re-allocated the ring buffer on the
> > fly(to change its size)? Would this done by re-calling
> > dc1394_dma_setup_capture?
>=20
> No, I don't think that would work.  What I meant was that a user may=20
> change their mind after initially setting up with=20
> dma_extra_buffer_enable=3D0 (so that no memory gets allocated to=20
> dma_extra_buffer which is set to a null pointer).  If=20
> dma_extra_buffer_enable is set some time after initialization/setup=20
> then dc1394_dma_multi_capture() needs to detect this and quickly=20
> allocate some memory to dma_extra_buffer (preferably by calling the=20
> new proposed fundtion _dc1394_alloc_extra_buffer()) before=20
> memcpy(dma_extra_buffer,...) tries to access the null pointer.

Before saying any more stupid things I guess I'll have a deeper look at
the code to understand what this extra buffer really is... ;-)

Damien

--=20
  _    Damien Douxchamps
 (=B0-   PhD Student / Research Assistant
 //\   Image Processing Group, Telecom Laboratory, UCL, Belgium
 V_/_  http://www.tele.ucl.ac.be/MEMBERS/Douxchamps_Damien_e.html

On Mon, 2003-11-24 at 05:16, Damien Douxchamps wrote:
> Hi Johann,
> 
> On Sun, 2003-11-23 at 22:44, Johann Schoonees wrote:
> > >>  4. Add a new flag member to the dc1394_cameracapture stucture called 
> > >>something like dma_extra_buffer_enable which switches copying to 
> > >>dma_extra_buffer on or off.  (We will have to decide what to do if 
> > >>both drop_frames and dma_extra_buffer_enable flags are set.)

This parameter is not necessary because this behavior is directly
related to the drop_frames behavior. The latter condition of both
enabling drop_frames and a new dma_extra_buffer flag is a complete
contradiction.

> > > I don't understand what will have to be decided. Please give me more
> > > details.
> > 
> > dma_extra_buffer_enable!=0 means don't drop frames if possible but 
> > buffer those by which we are behind.
> >
> > drop_frames!=0 means drop all frames by which we are behind.
> > 
> > It is possible that the user can set both flags.  Then do we drop 
> > frames or not?  We will have to decide which flag has precedence if 
> > both are set simultaneously.  My inclination is to play it safe and do 
> > the buffering, i.e. dma_extra_buffer_enable has precedence.
> > 
> > This is only a preliminary idea for the dma_extra_buffer_enable flag. 
> >   If anyone can think of a different flag that avoids the conflict 
> > (and breaks a minimum of existing code), please speak up.
> 
> I realize I've never quite understood the purpose of the extra_buffer!
> ;-) A quick reading of dc1394_capture didn't help but I still have two
> comments:

The purpose is to minimize the chance of dropping a frame if it is
important to try to capture every frame.

> - is it really interesting to have a multi_capture function? Wouldn't a
> single capture function be enough? 

Actually, it is more the other way around. Single capture is simply
multi capture called one iteration.

> The comments in dc1394_capture (line
> 325) seem to indicate that the extra_buffer is only for multi capture,
> so removing this feature might remove the problem. I personally think
> that it's very easy to use a "for (each camera on the bus) do capture
> end" so I don't really see the point of this multi capture at all. Then
> again, I did not understand all the code ;)

Multi capture takes care of switching between multiple cameras across
multiple 1394 buses; something that is not trivial.

> - if the parameter is meaningful, then I would suggest to use some enum
> type to limit the possibilities to the 3 meaningful ones: no drop/no
> extra buffer, drop/no extra buffer and no drop/extra buffer. It could
> still be a bit mask but it would be on 2 bits.

I do not agree with this change.
"drop/no extra buffer" and "no drop/extra buffer" is the current
behaviour.
"no drop/no extra buffer" does not make any sense because what is one
supposed to do with the extra frames you receive? After all, you
requested no dropping of frames. The only reason it might make sense is
to completely change the capture model within libdc1394.

> > >>  6. Allocate memory for dma_extra_buffer in 
> > >>dc1394_dma_multi_capture() if 
> > >>dma_extra_buffer_enable&&dma_extra_buffer==NULL.  That enables the 
> > >>user to request extra buffers by modifying dc1394_cameracapture on the 
> > >>fly.  (Currently this could cause a segfault.)
> > > 
> > > 
> > > So in short this would let the user re-allocated the ring buffer on the
> > > fly(to change its size)? Would this done by re-calling
> > > dc1394_dma_setup_capture?
> > 
> > No, I don't think that would work.  What I meant was that a user may 
> > change their mind after initially setting up with 
> > dma_extra_buffer_enable=0 (so that no memory gets allocated to 
> > dma_extra_buffer which is set to a null pointer).  If 
> > dma_extra_buffer_enable is set some time after initialization/setup 
> > then dc1394_dma_multi_capture() needs to detect this and quickly 
> > allocate some memory to dma_extra_buffer (preferably by calling the 
> > new proposed fundtion _dc1394_alloc_extra_buffer()) before 
> > memcpy(dma_extra_buffer,...) tries to access the null pointer.

If someone wants to change their capture settings after capture is
started, they must stop capture, make the change, and re-start. Changing
this on the fly is stupid and will make it needlessly complex.

IMO, the only legitimate problems are to avoid segfault and those
related to filltime. To be honest, filltime was something first added to
video1394 outside the libdc1394 project. Problems with filltime are also
in video1394 because it has no way to set the filltime for each buffer
of multiple frames returned.

I am wondering if I can update libdc1394 to use the new raw1394_iso API.
There is a param on the iso_receive callback that provide some cycle
time information, but I have to analyse further if it is enough. 

> Before saying any more stupid things I guess I'll have a deeper look at
> the code to understand what this extra buffer really is... ;-)

It is really quite simple. If you choose to not drop frames, then what
should be done with those potentially extra frames? Why, buffer them, of
course.

Hi Dan, Johann,

On Mon, 2003-11-24 at 15:12, Dan Dennedy wrote:
> On Mon, 2003-11-24 at 05:16, Damien Douxchamps wrote:
> > Hi Johann,
> >=20
> > On Sun, 2003-11-23 at 22:44, Johann Schoonees wrote:
> > > >>  4. Add a new flag member to the dc1394_cameracapture stucture cal=
led=20
> > > >>something like dma_extra_buffer_enable which switches copying to=20
> > > >>dma_extra_buffer on or off.  (We will have to decide what to do if=20
> > > >>both drop_frames and dma_extra_buffer_enable flags are set.)
>=20
> This parameter is not necessary because this behavior is directly
> related to the drop_frames behavior. The latter condition of both
> enabling drop_frames and a new dma_extra_buffer flag is a complete
> contradiction.

That was my understanding until Johann came ;)

> > > > I don't understand what will have to be decided. Please give me mor=
e
> > > > details.
> > >=20
> > > dma_extra_buffer_enable!=3D0 means don't drop frames if possible but=20
> > > buffer those by which we are behind.
> > >
> > > drop_frames!=3D0 means drop all frames by which we are behind.
> > >=20
> > > It is possible that the user can set both flags.  Then do we drop=20
> > > frames or not?  We will have to decide which flag has precedence if=20
> > > both are set simultaneously.  My inclination is to play it safe and d=
o=20
> > > the buffering, i.e. dma_extra_buffer_enable has precedence.
> > >=20
> > > This is only a preliminary idea for the dma_extra_buffer_enable flag.=
=20
> > >   If anyone can think of a different flag that avoids the conflict=20
> > > (and breaks a minimum of existing code), please speak up.
> >=20
> > I realize I've never quite understood the purpose of the extra_buffer!
> > ;-) A quick reading of dc1394_capture didn't help but I still have two
> > comments:
>=20
> The purpose is to minimize the chance of dropping a frame if it is
> important to try to capture every frame.
>=20
> > - is it really interesting to have a multi_capture function? Wouldn't a
> > single capture function be enough?=20
>=20
> Actually, it is more the other way around. Single capture is simply
> multi capture called one iteration.
>=20
> > The comments in dc1394_capture (line
> > 325) seem to indicate that the extra_buffer is only for multi capture,
> > so removing this feature might remove the problem. I personally think
> > that it's very easy to use a "for (each camera on the bus) do capture
> > end" so I don't really see the point of this multi capture at all. Then
> > again, I did not understand all the code ;)
>=20
> Multi capture takes care of switching between multiple cameras across
> multiple 1394 buses; something that is not trivial.

If multiple capture is not simply a loop on a single capture procedure,
we should definitely keep it.

> > - if the parameter is meaningful, then I would suggest to use some enum
> > type to limit the possibilities to the 3 meaningful ones: no drop/no
> > extra buffer, drop/no extra buffer and no drop/extra buffer. It could
> > still be a bit mask but it would be on 2 bits.
>=20
> I do not agree with this change.
> "drop/no extra buffer" and "no drop/extra buffer" is the current
> behaviour.
> "no drop/no extra buffer" does not make any sense because what is one
> supposed to do with the extra frames you receive? After all, you
> requested no dropping of frames. The only reason it might make sense is
> to completely change the capture model within libdc1394.

This is my point of view too.

> > > >>  6. Allocate memory for dma_extra_buffer in=20
> > > >>dc1394_dma_multi_capture() if=20
> > > >>dma_extra_buffer_enable&&dma_extra_buffer=3D=3DNULL.  That enables =
the=20
> > > >>user to request extra buffers by modifying dc1394_cameracapture on =
the=20
> > > >>fly.  (Currently this could cause a segfault.)
> > > >=20
> > > >=20
> > > > So in short this would let the user re-allocated the ring buffer on=
 the
> > > > fly(to change its size)? Would this done by re-calling
> > > > dc1394_dma_setup_capture?
> > >=20
> > > No, I don't think that would work.  What I meant was that a user may=20
> > > change their mind after initially setting up with=20
> > > dma_extra_buffer_enable=3D0 (so that no memory gets allocated to=20
> > > dma_extra_buffer which is set to a null pointer).  If=20
> > > dma_extra_buffer_enable is set some time after initialization/setup=20
> > > then dc1394_dma_multi_capture() needs to detect this and quickly=20
> > > allocate some memory to dma_extra_buffer (preferably by calling the=20
> > > new proposed fundtion _dc1394_alloc_extra_buffer()) before=20
> > > memcpy(dma_extra_buffer,...) tries to access the null pointer.
>=20
> If someone wants to change their capture settings after capture is
> started, they must stop capture, make the change, and re-start. Changing
> this on the fly is stupid and will make it needlessly complex.

I'm wondering which code sequence would create such segfault. This kind
of error never happened to me.

> IMO, the only legitimate problems are to avoid segfault and those
> related to filltime. To be honest, filltime was something first added to
> video1394 outside the libdc1394 project. Problems with filltime are also
> in video1394 because it has no way to set the filltime for each buffer
> of multiple frames returned.
>=20
> I am wondering if I can update libdc1394 to use the new raw1394_iso API.
> There is a param on the iso_receive callback that provide some cycle
> time information, but I have to analyse further if it is enough.=20

Is it correct that this upgrade to the new ISO API would make video1394
obsolete and that the capture would perform better? In that case this is
the one big step forward we should make in libdc1394 capture.

> > Before saying any more stupid things I guess I'll have a deeper look at
> > the code to understand what this extra buffer really is... ;-)
>=20
> It is really quite simple. If you choose to not drop frames, then what
> should be done with those potentially extra frames? Why, buffer them, of
> course.

I was confused with by difference between dma_ring_buffer and
dma_extra_buffer. I feel better now ;-)

Damien


--=20
  _    Damien Douxchamps
 (=B0-   PhD Student / Research Assistant
 //\   Image Processing Group, Telecom Laboratory, UCL, Belgium
 V_/_  http://www.tele.ucl.ac.be/MEMBERS/Douxchamps_Damien_e.html

On Mon, 2003-11-24 at 11:11, Damien Douxchamps wrote:
> > I am wondering if I can update libdc1394 to use the new raw1394_iso API.
> > There is a param on the iso_receive callback that provide some cycle
> > time information, but I have to analyse further if it is enough. 
> 
> Is it correct that this upgrade to the new ISO API would make video1394
> obsolete and that the capture would perform better? In that case this is
> the one big step forward we should make in libdc1394 capture.

Yes, it would make video1394 obsolete with roughly the same capture
performance; it is hard to better on the performance of video1394. It
would simplify eveything, like having to make sure the additional module
is loaded and device file names. Also, it is the intention to eventually
deprecate ohci-specific modules such as video1394 (and dv1394) since the
new API interfaces better with the ieee1394 subsystem.

Hi Dan, Damien

I'll reply to both your emails together.

Dan Dennedy wrote:
> On Mon, 2003-11-24 at 05:16, Damien Douxchamps wrote:
> 
>>Hi Johann,
>>
>>On Sun, 2003-11-23 at 22:44, Johann Schoonees wrote:
>>
>>>>> 4. Add a new flag member to the dc1394_cameracapture stucture called 
>>>>>something like dma_extra_buffer_enable which switches copying to 
>>>>>dma_extra_buffer on or off.  (We will have to decide what to do if 
>>>>>both drop_frames and dma_extra_buffer_enable flags are set.)
> 
> 
> This parameter is not necessary because this behavior is directly
> related to the drop_frames behavior. The latter condition of both
> enabling drop_frames and a new dma_extra_buffer flag is a complete
> contradiction.

I have found it necessary in my application to be able to do neither. 
  In other words, I do not want to drop frames and I do not want to 
buffer them either.  Mostly, I just want to know by how many frames I 
am behind in the DMA ring buffer and be able to flush them if I can't 
catch up.

I get the impression that most of the libdc1394 development is done 
with desktop PCs in mind, perhaps to support webcams or the like.  In 
more stringent environments (embedded processors doing image 
processing and other jobs in real time) one cannot always afford to 
have the dc1394_dma_multi_capture() function do an unexpected memcpy() 
to an extra buffer if the timing is tight.

I think my application is fairly representative of such situations:  I 
do not want to lose any frames if possible, but I also cannot afford 
the overhead of extra buffering.  My interface to the rest of the 
system is specified to say: if I can't keep up with my processing 
load, then I may drop a frame as long as I tell the system which one I 
dropped (for example by a frame number or a gap in a series of frame 
numbers).

As it is currently implemented, buffering is done in a bursty way.  In 
systems which have to synchronize processing with regular external 
events it leads to situations where, once a frame gets copied to 
extra_buffer, it triggers a cascade of more extra_buffer copying and 
may not catch up again.  The reason for such behaviour is that while 
dc1394_dma_multi_cpature() returns frames from dma_extra_buffer (i.e. 
while dma_extra_count>0), frames heap up in the DMA ring buffer.

So, for me (and presumably others who do real-time processing) it is 
not just a simple drop_frames/extra_buffer decision.  I need more 
information about how full the DMA buffer is and finer control over 
when to flush frames from the ring buffer.

>>- if the parameter is meaningful, then I would suggest to use some enum
>>type to limit the possibilities to the 3 meaningful ones: no drop/no
>>extra buffer, drop/no extra buffer and no drop/extra buffer. It could
>>still be a bit mask but it would be on 2 bits.
> 
> 
> I do not agree with this change.
> "drop/no extra buffer" and "no drop/extra buffer" is the current
> behaviour.
> "no drop/no extra buffer" does not make any sense because what is one
> supposed to do with the extra frames you receive? After all, you
> requested no dropping of frames. The only reason it might make sense is
> to completely change the capture model within libdc1394.

I understand the original intention of drop_frames.  But as I 
explained above, a "no drop/no extra buffer" would make a lot of sense 
in my application.  Especially if it is accompanied by feedback on the 
number of frames by which we are behind in the DMA ring buffer.

I do not think adding this new functionality will change the capture 
model within libdc1394 much.  The only change would be for existing 
code to add a line like dma_extra_buffer_enable=1 to the existing 
drop_frames=0.

I agree that the meaning of drop_frames changes slightly.  It used to 
mean (and may continue like that if we decide not to make the proposed 
changes) "IF drop_frames, flush frames from the DMA ring buffer if we 
fall behind, ELSE buffer them in user space."  With the proposed 
change its meaning would become "IF drop_frames, flush frames from the 
DMA ring buffer if we fall behind, ELSE leave them alone."

All dma_extra_buffer_enable does is to allow the separation of the 
quite different concepts of flushing and buffering.  Which makes me 
think, if we do go ahead with such a change, would it be better to 
rename drop_frames to flush_frames?

>>>>> 6. Allocate memory for dma_extra_buffer in 
>>>>>dc1394_dma_multi_capture() if 
>>>>>dma_extra_buffer_enable&&dma_extra_buffer==NULL.  That enables the 
>>>>>user to request extra buffers by modifying dc1394_cameracapture on the 
>>>>>fly.  (Currently this could cause a segfault.)
>>>>
>>>>
>>>>So in short this would let the user re-allocated the ring buffer on the
>>>>fly(to change its size)? Would this done by re-calling
>>>>dc1394_dma_setup_capture?
>>>
>>>No, I don't think that would work.  What I meant was that a user may 
>>>change their mind after initially setting up with 
>>>dma_extra_buffer_enable=0 (so that no memory gets allocated to 
>>>dma_extra_buffer which is set to a null pointer).  If 
>>>dma_extra_buffer_enable is set some time after initialization/setup 
>>>then dc1394_dma_multi_capture() needs to detect this and quickly 
>>>allocate some memory to dma_extra_buffer (preferably by calling the 
>>>new proposed fundtion _dc1394_alloc_extra_buffer()) before 
>>>memcpy(dma_extra_buffer,...) tries to access the null pointer.
> 
> 
> If someone wants to change their capture settings after capture is
> started, they must stop capture, make the change, and re-start. Changing
> this on the fly is stupid and will make it needlessly complex.

I don't mind either way.  This is not a high priority for me.

My original thinking was that the user (as opposed to the developer) 
sees a certain view of the libdc1394 interface and, hey! here is a 
flag called frop_frames (or dma_extra_buffer_enable or whatever) what 
happens if I change it?  The user should not have to worry about 
details like memory allocation and whether they need to restart when 
some flags are changed but not others.  All the typical user cares 
about is high-level functionality like "can I do buffering and where 
do I switch it on and off."  The low-level code can take care of the rest.

Regards,
Johann

PS: I have written a little wrapper around libdc1394 (actually it is 
not so little: 2800 lines) which defines an implementation-independent 
interface to a generic camera.  The user need not know that the bus is 
IEEE 1394.  Exactly the same interface can be used for USB cameras, 
CameraLink, etc. by writing the appropriate code behind the interface. 
  It has high-level general functions like cam_create(), 
cam_destroy(), cam_get/set_frame_dims(), cam_get/set_framerate(), 
cam_get/set_trigger_source(), cam_get/set_single_shot(), 
cam_get/set_run_stop(), cam_point_to_next_frame(), cam_unpoint_frame() 
and so on.  Anything (even frame dimensions and frame rates) can be 
changed at any time, even while the camera is running.  No need for 
the user to know anything about the constraints of IEEE 1394 or 
libdc1394.  Also an example program which uses the interface.  Some 
day I'll tidy it up and put it somewhere accessible.  J.

-- 
^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^
Johann Schoonees                         Imaging & Sensing Team
Industrial Research Limited, PO Box 2225, Auckland, New Zealand
Phone +64 9 9203679 Fax +64 9 3028106 http://www.is.irl.cri.nz/

Dan Dennedy wrote:
> 
> IMO, the only legitimate problems are to avoid segfault and those
> related to filltime. To be honest, filltime was something first added to
> video1394 outside the libdc1394 project. Problems with filltime are also
> in video1394 because it has no way to set the filltime for each buffer
> of multiple frames returned.

This is relevant to my system.  We need accurate filltimes for every 
frame from every camera.  Could you give more detail on why/how 
video1394 does not set filltime for multiple frames?

I find the video1394 code quite hard to read (I haven't done any 
kernel/driver programming).  As far as I can see the relevant bits of 
video1394.c are:

void wakeup_dma_ir_ctx(unsigned long l)
{
	struct dma_iso_ctx *d = (struct dma_iso_ctx *) l;
	int i;

	spin_lock(&d->lock);

	for (i = 0; i < d->num_desc; i++) {
		if (d->ir_prg[i][d->nb_cmd-1].status & cpu_to_le32(0xFFFF0000)) {
			reset_ir_status(d, i);
			d->buffer_status[i] = VIDEO1394_BUFFER_READY;
			do_gettimeofday(&d->buffer_time[i]);
		}
	}

	spin_unlock(&d->lock);

	if (waitqueue_active(&d->waitq))
		wake_up_interruptible(&d->waitq);
}
...
static int video1394_ioctl(struct inode *inode, struct file *file,
			   unsigned int cmd, unsigned long arg)
{
	...
	switch(cmd)
	{
	...
	case VIDEO1394_LISTEN_WAIT_BUFFER:
	case VIDEO1394_LISTEN_POLL_BUFFER:
	{
		struct video1394_wait v;
		struct dma_iso_ctx *d;
		...
		/* set time of buffer */
		v.filltime = d->buffer_time[v.buffer];
		...
		return 0;
	}
	...
	}
}


It seems there are num_desc ISO descriptors whatever those are, one 
for each channel or camera.  An ISO DMA context gets allocated every 
time ioctl() is called with VIDEO1394_LISTEN_CHANNEL.  Whenever a 
buffer is filled (is that what if (d->ir_prg[i][d->nb_cmd-1].status & 
cpu_to_le32(0xFFFF0000)) means?) the corresponding buffer_time is 
updated with do_gettimeofday() and returned as a filltime.

Have I missed something?

Regards,
Johann
-- 
^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^
Johann Schoonees                         Imaging & Sensing Team
Industrial Research Limited, PO Box 2225, Auckland, New Zealand
Phone +64 9 9203679 Fax +64 9 3028106 http://www.is.irl.cri.nz/

Ah, thanks for setting me straight on the requirement for the
*application* to control when frames are buffered vs. dropped. That does
make sense. 

On Tue, 2003-11-25 at 06:20, Johann Schoonees wrote:
> Hi Dan, Damien
> 
> I'll reply to both your emails together.
> 
> Dan Dennedy wrote:
> > On Mon, 2003-11-24 at 05:16, Damien Douxchamps wrote:
> > 
> >>Hi Johann,
> >>
> >>On Sun, 2003-11-23 at 22:44, Johann Schoonees wrote:
> >>
> >>>>> 4. Add a new flag member to the dc1394_cameracapture stucture called 
> >>>>>something like dma_extra_buffer_enable which switches copying to 
> >>>>>dma_extra_buffer on or off.  (We will have to decide what to do if 
> >>>>>both drop_frames and dma_extra_buffer_enable flags are set.)
> > 
> > 
> > This parameter is not necessary because this behavior is directly
> > related to the drop_frames behavior. The latter condition of both
> > enabling drop_frames and a new dma_extra_buffer flag is a complete
> > contradiction.
> 
> I have found it necessary in my application to be able to do neither. 
>   In other words, I do not want to drop frames and I do not want to 
> buffer them either.  Mostly, I just want to know by how many frames I 
> am behind in the DMA ring buffer and be able to flush them if I can't 
> catch up.
> 
> I get the impression that most of the libdc1394 development is done 
> with desktop PCs in mind, perhaps to support webcams or the like.  In 
> more stringent environments (embedded processors doing image 
> processing and other jobs in real time) one cannot always afford to 
> have the dc1394_dma_multi_capture() function do an unexpected memcpy() 
> to an extra buffer if the timing is tight.
> 
> I think my application is fairly representative of such situations:  I 
> do not want to lose any frames if possible, but I also cannot afford 
> the overhead of extra buffering.  My interface to the rest of the 
> system is specified to say: if I can't keep up with my processing 
> load, then I may drop a frame as long as I tell the system which one I 
> dropped (for example by a frame number or a gap in a series of frame 
> numbers).
> 
> As it is currently implemented, buffering is done in a bursty way.  In 
> systems which have to synchronize processing with regular external 
> events it leads to situations where, once a frame gets copied to 
> extra_buffer, it triggers a cascade of more extra_buffer copying and 
> may not catch up again.  The reason for such behaviour is that while 
> dc1394_dma_multi_cpature() returns frames from dma_extra_buffer (i.e. 
> while dma_extra_count>0), frames heap up in the DMA ring buffer.
> 
> So, for me (and presumably others who do real-time processing) it is 
> not just a simple drop_frames/extra_buffer decision.  I need more 
> information about how full the DMA buffer is and finer control over 
> when to flush frames from the ring buffer.
> 
> >>- if the parameter is meaningful, then I would suggest to use some enum
> >>type to limit the possibilities to the 3 meaningful ones: no drop/no
> >>extra buffer, drop/no extra buffer and no drop/extra buffer. It could
> >>still be a bit mask but it would be on 2 bits.
> > 
> > 
> > I do not agree with this change.
> > "drop/no extra buffer" and "no drop/extra buffer" is the current
> > behaviour.
> > "no drop/no extra buffer" does not make any sense because what is one
> > supposed to do with the extra frames you receive? After all, you
> > requested no dropping of frames. The only reason it might make sense is
> > to completely change the capture model within libdc1394.
> 
> I understand the original intention of drop_frames.  But as I 
> explained above, a "no drop/no extra buffer" would make a lot of sense 
> in my application.  Especially if it is accompanied by feedback on the 
> number of frames by which we are behind in the DMA ring buffer.
> 
> I do not think adding this new functionality will change the capture 
> model within libdc1394 much.  The only change would be for existing 
> code to add a line like dma_extra_buffer_enable=1 to the existing 
> drop_frames=0.
> 
> I agree that the meaning of drop_frames changes slightly.  It used to 
> mean (and may continue like that if we decide not to make the proposed 
> changes) "IF drop_frames, flush frames from the DMA ring buffer if we 
> fall behind, ELSE buffer them in user space."  With the proposed 
> change its meaning would become "IF drop_frames, flush frames from the 
> DMA ring buffer if we fall behind, ELSE leave them alone."
> 
> All dma_extra_buffer_enable does is to allow the separation of the 
> quite different concepts of flushing and buffering.  Which makes me 
> think, if we do go ahead with such a change, would it be better to 
> rename drop_frames to flush_frames?
> 
> >>>>> 6. Allocate memory for dma_extra_buffer in 
> >>>>>dc1394_dma_multi_capture() if 
> >>>>>dma_extra_buffer_enable&&dma_extra_buffer==NULL.  That enables the 
> >>>>>user to request extra buffers by modifying dc1394_cameracapture on the 
> >>>>>fly.  (Currently this could cause a segfault.)
> >>>>
> >>>>
> >>>>So in short this would let the user re-allocated the ring buffer on the
> >>>>fly(to change its size)? Would this done by re-calling
> >>>>dc1394_dma_setup_capture?
> >>>
> >>>No, I don't think that would work.  What I meant was that a user may 
> >>>change their mind after initially setting up with 
> >>>dma_extra_buffer_enable=0 (so that no memory gets allocated to 
> >>>dma_extra_buffer which is set to a null pointer).  If 
> >>>dma_extra_buffer_enable is set some time after initialization/setup 
> >>>then dc1394_dma_multi_capture() needs to detect this and quickly 
> >>>allocate some memory to dma_extra_buffer (preferably by calling the 
> >>>new proposed fundtion _dc1394_alloc_extra_buffer()) before 
> >>>memcpy(dma_extra_buffer,...) tries to access the null pointer.
> > 
> > 
> > If someone wants to change their capture settings after capture is
> > started, they must stop capture, make the change, and re-start. Changing
> > this on the fly is stupid and will make it needlessly complex.
> 
> I don't mind either way.  This is not a high priority for me.
> 
> My original thinking was that the user (as opposed to the developer) 
> sees a certain view of the libdc1394 interface and, hey! here is a 
> flag called frop_frames (or dma_extra_buffer_enable or whatever) what 
> happens if I change it?  The user should not have to worry about 
> details like memory allocation and whether they need to restart when 
> some flags are changed but not others.  All the typical user cares 
> about is high-level functionality like "can I do buffering and where 
> do I switch it on and off."  The low-level code can take care of the rest.
> 
> Regards,
> Johann
> 
> PS: I have written a little wrapper around libdc1394 (actually it is 
> not so little: 2800 lines) which defines an implementation-independent 
> interface to a generic camera.  The user need not know that the bus is 
> IEEE 1394.  Exactly the same interface can be used for USB cameras, 
> CameraLink, etc. by writing the appropriate code behind the interface. 
>   It has high-level general functions like cam_create(), 
> cam_destroy(), cam_get/set_frame_dims(), cam_get/set_framerate(), 
> cam_get/set_trigger_source(), cam_get/set_single_shot(), 
> cam_get/set_run_stop(), cam_point_to_next_frame(), cam_unpoint_frame() 
> and so on.  Anything (even frame dimensions and frame rates) can be 
> changed at any time, even while the camera is running.  No need for 
> the user to know anything about the constraints of IEEE 1394 or 
> libdc1394.  Also an example program which uses the interface.  Some 
> day I'll tidy it up and put it somewhere accessible.  J.

On Tue, 2003-11-25 at 07:22, Johann Schoonees wrote:
> Dan Dennedy wrote:

> > video1394 outside the libdc1394 project. Problems with filltime are also
> > in video1394 because it has no way to set the filltime for each buffer
> > of multiple frames returned.
> 
> This is relevant to my system.  We need accurate filltimes for every 
> frame from every camera.  Could you give more detail on why/how 
> video1394 does not set filltime for multiple frames?
[...]
> static int video1394_ioctl(struct inode *inode, struct file *file,
> 			   unsigned int cmd, unsigned long arg)
> {
> 	...
> 	switch(cmd)
> 	{
> 	...
> 	case VIDEO1394_LISTEN_WAIT_BUFFER:
> 	case VIDEO1394_LISTEN_POLL_BUFFER:
> 	{
> 		struct video1394_wait v;
> 		struct dma_iso_ctx *d;
> 		...
> 		/* set time of buffer */
> 		v.filltime = d->buffer_time[v.buffer];

At this point additional buffers beyond v.buffer are possibly ready and
returned in the video1394_wait struct. However, the struct only holds
one filltime value rather an array of values for each frame. Also, if we
are dropping frames, then filltime represents the wrong frame--the
earliest frame of the sequence rather than the latest.

> It seems there are num_desc ISO descriptors whatever those are, one 
> for each channel or camera. 

yes, the iso decriptors are basically the "statements" of the dma
"program."

> An ISO DMA context gets allocated every 
> time ioctl() is called with VIDEO1394_LISTEN_CHANNEL.

yes

>  Whenever a 
> buffer is filled (is that what if (d->ir_prg[i][d->nb_cmd-1].status & 
> cpu_to_le32(0xFFFF0000)) means?) the corresponding buffer_time is 
> updated with do_gettimeofday() and returned as a filltime.

yes

Dan Dennedy wrote:
> On Tue, 2003-11-25 at 07:22, Johann Schoonees wrote:
> 
>>Dan Dennedy wrote:
> 
> 
>>>video1394 outside the libdc1394 project. Problems with filltime are also
>>>in video1394 because it has no way to set the filltime for each buffer
>>>of multiple frames returned.
>>
>>This is relevant to my system.  We need accurate filltimes for every 
>>frame from every camera.  Could you give more detail on why/how 
>>video1394 does not set filltime for multiple frames?
> 
> [...]
> 
>>static int video1394_ioctl(struct inode *inode, struct file *file,
>>			   unsigned int cmd, unsigned long arg)
>>{
>>	...
>>	switch(cmd)
>>	{
>>	...
>>	case VIDEO1394_LISTEN_WAIT_BUFFER:
>>	case VIDEO1394_LISTEN_POLL_BUFFER:
>>	{
>>		struct video1394_wait v;
>>		struct dma_iso_ctx *d;
>>		...
>>		/* set time of buffer */
>>		v.filltime = d->buffer_time[v.buffer];
> 
> 
> At this point additional buffers beyond v.buffer are possibly ready and
> returned in the video1394_wait struct

Not exactly.  Correct me if I'm wrong, but I think the *number* of 
buffers by which we are behind is returned in the video1394_wait 
struct, not the buffers themselves.  dc1394_dma_multi_capture() uses 
this number to flush frames from the DMA ring buffer by calling 
ioctl(...,VIDEO1394_LISTEN_QUEUE_BUFFER) for each buffer to be 
flushed.  Before this is done, each of those waiting buffers should 
hopefully have its filltime still intact in the buffer_time array in 
video1394, no?.  The filltimes are only finally lost when we call 
ioctl(...,VIDEO1394_LISTEN_QUEUE_BUFFER), at least that is the picture 
I have in mind.

> However, the struct only holds
> one filltime value rather an array of values for each frame. Also, if we
> are dropping frames, then filltime represents the wrong frame--the
> earliest frame of the sequence rather than the latest.

I agree that libdc1394 currently handles filltimes the way you 
descibe.  But it does not *have* to do that.  I am guessing the 
correct filltime can be returned by polling video1394 for the latest 
known frame after the earlier ones had been flushed.  One could add 
the following after the for(j=0;j<extra_buf;j++) loop that calls 
ioctl(...,VIDEO1394_LISTEN_QUEUE_BUFFER):

        /* Get the corresponding filltime: */
        vwait.buffer = cams[i].dma_last_buffer;
        ioctl_retval = ioctl(cams[i].dma_fd,
                VIDEO1394_LISTEN_POLL_BUFFER,
                &vwait);
        if (ioctl_retval != 0)  return DC1394_FAILURE;
        cams[i].filltime = vwait.filltime;

I currently do this in my application and it seems to work although I 
have not had a chance to verify that it really is the right filltime.

Regards,
Johann
-- 
^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^
Johann Schoonees                         Imaging & Sensing Team
Industrial Research Limited, PO Box 2225, Auckland, New Zealand
Phone +64 9 9203679 Fax +64 9 3028106 http://www.is.irl.cri.nz/

On Tue, 2003-11-25 at 11:19, Johann Schoonees wrote:
> Dan Dennedy wrote:
> > On Tue, 2003-11-25 at 07:22, Johann Schoonees wrote:
> > 
> >>Dan Dennedy wrote:
> > 
> > 
> >>>video1394 outside the libdc1394 project. Problems with filltime are also
> >>>in video1394 because it has no way to set the filltime for each buffer
> >>>of multiple frames returned.
> >>
> >>This is relevant to my system.  We need accurate filltimes for every 
> >>frame from every camera.  Could you give more detail on why/how 
> >>video1394 does not set filltime for multiple frames?
> > 
> > [...]
> > 
> >>static int video1394_ioctl(struct inode *inode, struct file *file,
> >>			   unsigned int cmd, unsigned long arg)
> >>{
> >>	...
> >>	switch(cmd)
> >>	{
> >>	...
> >>	case VIDEO1394_LISTEN_WAIT_BUFFER:
> >>	case VIDEO1394_LISTEN_POLL_BUFFER:
> >>	{
> >>		struct video1394_wait v;
> >>		struct dma_iso_ctx *d;
> >>		...
> >>		/* set time of buffer */
> >>		v.filltime = d->buffer_time[v.buffer];
> > 
> > 
> > At this point additional buffers beyond v.buffer are possibly ready and
> > returned in the video1394_wait struct
> 
> Not exactly.  Correct me if I'm wrong, but I think the *number* of 
> buffers by which we are behind is returned in the video1394_wait 
> struct, not the buffers themselves. 

Of course, because the buffers are mmap-ed, buffers are not physically
returned via copy.

>  dc1394_dma_multi_capture() uses 
> this number to flush frames from the DMA ring buffer by calling 
> ioctl(...,VIDEO1394_LISTEN_QUEUE_BUFFER) for each buffer to be 
> flushed.  Before this is done, each of those waiting buffers should 
> hopefully have its filltime still intact in the buffer_time array in 
> video1394, no?.  The filltimes are only finally lost when we call 
> ioctl(...,VIDEO1394_LISTEN_QUEUE_BUFFER), at least that is the picture 
> I have in mind.

True, these fill times are inside video1394.

> > However, the struct only holds
> > one filltime value rather an array of values for each frame. Also, if we
> > are dropping frames, then filltime represents the wrong frame--the
> > earliest frame of the sequence rather than the latest.
> 
> I agree that libdc1394 currently handles filltimes the way you 
> descibe.  But it does not *have* to do that.  I am guessing the 
> correct filltime can be returned by polling video1394 for the latest 
> known frame after the earlier ones had been flushed.  One could add 
> the following after the for(j=0;j<extra_buf;j++) loop that calls 
> ioctl(...,VIDEO1394_LISTEN_QUEUE_BUFFER):
> 
>         /* Get the corresponding filltime: */
>         vwait.buffer = cams[i].dma_last_buffer;
>         ioctl_retval = ioctl(cams[i].dma_fd,
>                 VIDEO1394_LISTEN_POLL_BUFFER,
>                 &vwait);
>         if (ioctl_retval != 0)  return DC1394_FAILURE;
>         cams[i].filltime = vwait.filltime;

OK, I see what you are doing. Honestly, I have not really used POLL or
filltimes; the filltime handling of libdc1394 was contributed. I had no
idea the filltime was available by polling each extra READY buffer
because I had never needed to look nor is it obvious.

Looking at the code, yes, if you POLL on a buffer that is READY, then it
does return the filltime for that buffer. As a side effect, it sets the
buffer status to FREE. To be most proper, it seems as though the above
should be done before calling VIDEO1394_LISTEN_QUEUE_BUFFER. Correct me
if I am wrong. However, as long as you dont POLL the first buffer
returned, it should operate okay regardless.

> I currently do this in my application and it seems to work although I 
> have not had a chance to verify that it really is the right filltime.

It should be. 

> Regards,
> Johann

Now, I still have to review much of what you proposed to see what best
suits the libdc1394 interface.

Welcome to the list.

Filltime is an item that has been on my list of things to work on
for a long time. I have an application that needs to know the time a frame 
comes in for every frame, without droping any. So, I need:

1. filltime to be start of frame and not end.
2. filltime for each frame.

I've just not had the time to work on this yet. I've been working on 
finding out why I get random bus resets that don't occur in the lab but
do occur now that I've got a system 1500 miles away. (It looks like
something to do with a bad fiber converter, since replacing it has
reduced the occurances greatly... which I've just jinxed by talking
about.)

My route to solving this would be to make the frame returned into
a struct with a time as the first member and the data as the second.
To make the struct compatible with people who don't want the time,
I would make the frame pointer point to the data, and the time would
be a negative offset from that. And I'd also make the time a 64 bit
value, so prepare for y2038 issues. That's just how I'd do it. It
would be convenient to be able to pass one pointer to the frame that
has both the frame and time data without having to construct a new copy.

Thanks for looking into this, and I look forward to your patches.

I'm also looking at a camera (MicroPix C-1024 from CCD Direct) that
appears to have the time data in the first 4 bytes of the image, whic
isn't a perfect solution, but I'll live with losing the first four bytes
if I can get sub-cycle timing.

Dan Dennedy wrote:
> On Tue, 2003-11-25 at 11:19, Johann Schoonees wrote:

[snip]

>>known frame after the earlier ones had been flushed.  One could add 
>>the following after the for(j=0;j<extra_buf;j++) loop that calls 
>>ioctl(...,VIDEO1394_LISTEN_QUEUE_BUFFER):
>>
>>        /* Get the corresponding filltime: */
>>        vwait.buffer = cams[i].dma_last_buffer;
>>        ioctl_retval = ioctl(cams[i].dma_fd,
>>                VIDEO1394_LISTEN_POLL_BUFFER,
>>                &vwait);
>>        if (ioctl_retval != 0)  return DC1394_FAILURE;
>>        cams[i].filltime = vwait.filltime;
> 
> 
> OK, I see what you are doing. Honestly, I have not really used POLL or
> filltimes; the filltime handling of libdc1394 was contributed. I had no
> idea the filltime was available by polling each extra READY buffer
> because I had never needed to look nor is it obvious.
> 
> Looking at the code, yes, if you POLL on a buffer that is READY, then it
> does return the filltime for that buffer. As a side effect, it sets the
> buffer status to FREE. To be most proper, it seems as though the above
> should be done before calling VIDEO1394_LISTEN_QUEUE_BUFFER. Correct me
> if I am wrong. However, as long as you dont POLL the first buffer
> returned, it should operate okay regardless.

Yes, I agree about doing the polling before calling 
ioctl(...,VIDEO1394_LISTEN_QUEUE_BUFFER) which should happen in 
dc1394_dma_done_with_buffer().  Polling and waiting are both handled 
in the same switch/case statement in video1394 with only an if() 
statement differentiating their behaviour.  So if a frame is ready and 
waiting, VIDEO1394_LISTEN_POLL_BUFFER does exactly the same things as 
VIDEO1394_LISTEN_WAIT_BUFFER.

> Now, I still have to review much of what you proposed to see what best
> suits the libdc1394 interface.

Thanks again for looking at my stuff.  It would be great to get some 
of this functionality into the next release of the library.

Regards,
Johann

-- 
^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^
Johann Schoonees                         Imaging & Sensing Team
Industrial Research Limited, PO Box 2225, Auckland, New Zealand
Phone +64 9 9203679 Fax +64 9 3028106 http://www.is.irl.cri.nz/

John Stanley wrote:
> Welcome to the list.
> 
> Filltime is an item that has been on my list of things to work on
> for a long time. I have an application that needs to know the time a frame 
> comes in for every frame, without droping any. So, I need:
> 
> 1. filltime to be start of frame and not end.
> 2. filltime for each frame.

I also need the time at start of transmission.  In fact I need the 
time of the external trigger that started frame capture.  So far I 
have had to work backwards from the filltime by subtracting the frame 
transmission time, the average time the camera had to wait for the 
next bus cycle (the biggest source of uncertainty), the shutter 
integration time, and the delay between trigger and frame acquisition. 
  It's awkward but it does seem to work.

> I've just not had the time to work on this yet. I've been working on 
> finding out why I get random bus resets that don't occur in the lab but
> do occur now that I've got a system 1500 miles away. (It looks like
> something to do with a bad fiber converter, since replacing it has
> reduced the occurances greatly... which I've just jinxed by talking
> about.)
> 
> My route to solving this would be to make the frame returned into
> a struct with a time as the first member and the data as the second.
> To make the struct compatible with people who don't want the time,
> I would make the frame pointer point to the data, and the time would
> be a negative offset from that. And I'd also make the time a 64 bit
> value, so prepare for y2038 issues. That's just how I'd do it. It
> would be convenient to be able to pass one pointer to the frame that
> has both the frame and time data without having to construct a new copy.

It sounds like an excellent idea to have the frame and its time 
together.  I just don't know how you'll do it.  At the moment 
capture_buffer points to the memory mapped DMA ring buffer.  You won't 
be able to insert a filltime in from of it without risking the 
filltime being overwritten by a subsequent frame.  Nor do I think it's 
a good idea to write stuff into the DMA ring buffer.  It is mapped 
with read-only protection anyway.

I know next to nothing about video1394 but you could probably do it by 
patching video1394 to allocate extra buffer space for the filltime. 
The filltime would have to go at the end of the frame to maintain 
compatibility with existing code.

Another thing that worries me a bit is that different compilers may 
insert different amounts of padding between struct members (or even 
the same compiler with different option flags).  Accessing filltimes 
by a negative offset from a pointer is not likely to be portable or 
easily maintainable.

On the 64-bit time, I wouldn't bother doing something non-standard. 
POSIX.4 deals with the problem by assuming that time_t will be at 
least 64 bits in 2038.  That's 35 years from now.  35 years ago there 
were no PCs, there weren't even any micro-computers.  The Lunar Lander 
used magnetic core memory for its tiny brainlet.  If you wanted to do 
some simple sums you had to run them in FORTRAN on an expensive beast 
that filled most of an air-conditioned room, using punchcards or 
magnetic tape as your input.  Your print-out (saying that hours of 
processing was wasted because one of your punchcards was out of order) 
was brought to you the next day by priests in white labcoats.  You get 
the idea.  In 35 years from now those of us who are still alive will 
wonder how we ever got anything useful done on those awkward, 
unreliable, noisy boxes that took up half a desk, what were they 
called? oh yes now I remember, "PCs."  :-)

> Thanks for looking into this, and I look forward to your patches.
> 
> I'm also looking at a camera (MicroPix C-1024 from CCD Direct) that
> appears to have the time data in the first 4 bytes of the image, whic
> isn't a perfect solution, but I'll live with losing the first four bytes
> if I can get sub-cycle timing.

That's interesting.  We at one stage thought of patching video1394 to 
do exactly the same thing if it did not provide accurate filltimes. 
Turned out not to be necessary.

Regards,
Johann
-- 
^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^
Johann Schoonees                         Imaging & Sensing Team
Industrial Research Limited, PO Box 2225, Auckland, New Zealand
Phone +64 9 9203679 Fax +64 9 3028106 http://www.is.irl.cri.nz/

On Wed, 26 Nov 2003, Johann Schoonees wrote:

> I know next to nothing about video1394 but you could probably do it by 
> patching video1394 to allocate extra buffer space for the filltime. 
> The filltime would have to go at the end of the frame to maintain 
> compatibility with existing code.

If the frame pointer points to the data, then the time can be anywhere.
Puting it before the data means you don't have to know how big the frame 
is, you look 8 bytes ahead of the data and you get the time. 

> Another thing that worries me a bit is that different compilers may 
> insert different amounts of padding between struct members (or even 
> the same compiler with different option flags).  Accessing filltimes 
> by a negative offset from a pointer is not likely to be portable or 
> easily maintainable.

Actually, anything that is 32bit aligned is safe. I've run into lots of
differences mapping char-sized elements and how they are padded, but never
with longs. Maybe on a 64 bit system, the fill will be to 64 bits.
 
> On the 64-bit time, I wouldn't bother doing something non-standard. 
> POSIX.4 deals with the problem by assuming that time_t will be at 
> least 64 bits in 2038.  That's 35 years from now.

However, the tm struct is 2 longs -- seconds and microseconds.

John Stanley wrote:
> On Wed, 26 Nov 2003, Johann Schoonees wrote:
> 
> 
>>I know next to nothing about video1394 but you could probably do it by 
>>patching video1394 to allocate extra buffer space for the filltime. 
>>The filltime would have to go at the end of the frame to maintain 
>>compatibility with existing code.
> 
> 
> If the frame pointer points to the data, then the time can be anywhere.
> Puting it before the data means you don't have to know how big the frame 
> is, you look 8 bytes ahead of the data and you get the time. 

Hi John

I'm a bit mystified here. :-)  Where will the data be when you insert 
the filltime 8 bytes before the data?  In the DMA ring buffer?  If so, 
how do you insert the filltime?  If not, where is the data and how did 
it get there?

Regards,
Johann

-- 
^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^
Johann Schoonees                         Imaging & Sensing Team
Industrial Research Limited, PO Box 2225, Auckland, New Zealand
Phone +64 9 9203679 Fax +64 9 3028106 http://www.is.irl.cri.nz/