From: Radu B. R. <ru...@cs...> - 2007-01-31 21:25:09
|
Maybe it would be good to start putting things down as in what needs to be done in order to accomplish this (variable length arrays in the data structures). We had several chats last year about it, but maybe we can concretize them. I had a discussion with a fellow colleague from another university about it today, and he reminded me (nicely) that this is still a major disadvantage in/of Player. At least if we have a concrete list of "to do"'s, some of the developers will probably silently start to implement them in their spare time. After that we can plan for some interface upgrades and integrate things like stereo (or n-camera). Maybe this is not a bad thing for the entire project overall? (the to-do list I mean) Cheers, Radu. -- | Radu Bogdan Rusu | http://rbrusu.com/ | http://www9.cs.tum.edu/people/rusu/ | Intelligent Autonomous Systems | Technische Universitaet Muenchen |
From: Brian G. <br...@ge...> - 2007-01-31 21:54:44
|
On Jan 31, 2007, at 1:24 PM, Radu Bogdan Rusu wrote: > > Maybe it would be good to start putting things down as in what > needs to be done in order > to accomplish this (variable length arrays in the data structures). > We had several chats > last year about it, but maybe we can concretize them. I had a > discussion with a fellow > colleague from another university about it today, and he reminded > me (nicely) that this is > still a major disadvantage in/of Player. > > At least if we have a concrete list of "to do"'s, some of the > developers will probably > silently start to implement them in their spare time. After that we > can plan for some > interface upgrades and integrate things like stereo (or n-camera). > > Maybe this is not a bad thing for the entire project overall? (the > to-do list I mean) A capital idea. I've just enabled the Task Manager at SF.net, which I think will help us handle this. To get things started, I wrote a couple of tasks (variable-length array support, native Windows builds): https://sourceforge.net/pm/task.php? group_id=42445&group_project_id=51099&func=browse&set=open If you have suggestions for more tasks, send them in and I'll add them. If you'd like to take on a task, let me know and I'll happily assign it to you :) brian. |
From: Geoffrey B. <g....@au...> - 2007-02-03 07:07:22
|
I started having a look at variable length arrays today since it seemed like a good way to learn how the XDR parts of Player work. I'm using the method Brian suggested on the task manager page for it. So far I haven't done much, just got the playerxdrgen.py script to recognise pointers and find the _count variable. The main implementation issue, and the one that's always killed discussion in the past, seems to be where memory is allocated/deallocated. My understanding of the way the XDR stuff works is that a buffer is allocated for the packed data just before it is packed and sent over the transport. It's easy enough to allocate this buffer correctly for the size of a variable length array. So the issue is driver and client writers needing to allocate/deallocate. Drivers and clients would probably have to allocate the array themselves before publishing, sending requests, etc, and this is where memory leak issues start to appear. I can imagine that most drivers using an interface with a variable length array would be sending regular large chunks of data of a similar size (such as a video frame) and in this case could reuse the array for each new message they publish. Same for clients making regular requests. Otherwise they'll have to allocate and remember to deallocate the array in their local copy of the message themselves. Because drivers/clients may want to reuse a message structure, including any variable length arrays, after publishing a message, I don't think we can put deallocation of the array into the XDR packing function so that it gets deleted when the message is packed. But we could possibly add a check for if the array pointer is NULL for unpacking, and have the function allocate necessary memory in this case. Although in the case of clients, the buffer is already allocated to be max message size so no need to allocate anything there, and if the buffer to unpack into isn't zeroed the packing function may not allocate as necessary. Since structures lack intelligence we can't add constructors/destructors without forcing the entire interface system into C++, which probably isn't desirable. That's about as far as I've got in thinking about how to do this so far. Does anyone else have any comments? Geoff Brian Gerkey wrote: > On Jan 31, 2007, at 1:24 PM, Radu Bogdan Rusu wrote: > >> Maybe it would be good to start putting things down as in what >> needs to be done in order >> to accomplish this (variable length arrays in the data structures). >> We had several chats >> last year about it, but maybe we can concretize them. I had a >> discussion with a fellow >> colleague from another university about it today, and he reminded >> me (nicely) that this is >> still a major disadvantage in/of Player. >> >> At least if we have a concrete list of "to do"'s, some of the >> developers will probably >> silently start to implement them in their spare time. After that we >> can plan for some >> interface upgrades and integrate things like stereo (or n-camera). >> >> Maybe this is not a bad thing for the entire project overall? (the >> to-do list I mean) > > A capital idea. I've just enabled the Task Manager at SF.net, which > I think will help us handle this. > > To get things started, I wrote a couple of tasks (variable-length > array support, native Windows builds): > > https://sourceforge.net/pm/task.php? > group_id=42445&group_project_id=51099&func=browse&set=open > > If you have suggestions for more tasks, send them in and I'll add > them. If you'd like to take on a task, let me know and I'll happily > assign it to you :) > > brian. -- Robotics research group, University of Auckland http://www.ece.auckland.ac.nz/~gbig005/ |
From: Geoffrey B. <g....@au...> - 2007-02-04 08:04:26
Attachments:
playerxdrgen.py_patch
|
Following on from yesterday's messy (and sometimes incorrect) stream-of-conciousness, I've done some more work with this. I currently have a system where I can define a dynamically allocated array in an interface. It allocates memory automatically when unpacking messages. All other allocation and deletion needs to be managed manually at the moment. It's still subject to a few limitations: - There's a memory leak when drivers send a dynamic array. We'll get to that in a minute. - Dynamic arrays cannot be in embedded structures contained within a message. For example, if you have an array of structures as part of the message, those structures cannot contain dynamically allocated arrays. This is because of the way memory allocation is handled for unpacking received messages. The xdr filter function called for the embedded structure type doesn't know if it's encoding or decoding, and so doesn't know if it needs to allocate memory or not. I'm not sure how to get around this problem yet. - When an array is encoded with XDR, it seems to encode the pointer into the message as well, even though this is rather useless. It results in 4 extra bytes being sent. I'm not sure why this is happening, perhaps someone more familiar with XDR knows. It's pretty nasty at the moment. I've attached the necessary modifications to playerxdrgen.py as a patch for people to try out and improve. The main issue is, of course, when memory gets allocated and deleted. After a bit of thought, I've come up with the following occasions when it will need to happen (if I've missed any, let me know): A. When a message goes from client to driver: 1 - Client allocates local copy (handled by client writer). 2 - Client deletes local copy (because client is single-threaded for writing to server, no conflict with needing to wait till message gets packed). 3 - Unpacking function on server allocates (handled automatically). 4 - Somewhere on the server, once message has been handled, array needs to be deleted (handled by driver writer?). B. When a message goes from driver to client: 1 - Driver allocates local copy (handled by driver writer) 2 - Memory allocated by driver needs to be deleted after message is packed. Due to threading, message may not be packed as soon as Publish is called, so cannot delete after Publish returns (memory leak!). 3 - Unpacking function on client allocates (handled automatically). 4 - Client library deletes after handling message, or passes ownership on via returning to client program (handled either by client lib dev or client program dev). The problematic ones are the deletes: B.4 could be problematic if other transport layers are introduces which handle dynamic arrays better and so don't need the client library to delete manually. B.2 is the worst. Because of threading issues, the driver can't free the memory itself without risking a race condition, nor can it reuse that memory. The playertcp can't free it because it doesn't know how to (a message body is opaque to it, and, as far as it knows, static). What should probably happen here is that when Publish is called a deep copy of the message structure is made instead of a shallow copy (which works fine so far because we haven't had dynamically allocated memory to worry about), but because the inside of messages isn't known, this is difficult. A.4 shouldn't be too bad, but it's not that nice for drivers to have to free memory allocated elsewhere once they've handled a message. One possible way to manage all this deleting is created a FinishMessage() function or similar that, when called, will clean up a message after it's been used (either packed or handled). This function will need to know how to clean up each message, so a customised function for each message is necessary. However, rather than writing these, we could automatically generate them similar to the packing functions, and use a function table to look them up in FinishMessage() and call the appropriate one. Messages without any dynamically allocated memory would have an empty function, and if we inlined these they would disappear (optimising compilers should remove empty functions anyway). The function table lookup could be considered wasteful, though, when the bulk of messages don't need cleaning up. Auto-generation could also be used to create functions to handle deep copies of messages made when they get pushed onto message queues. Geoff Geoffrey Biggs wrote: > I started having a look at variable length arrays today since it seemed > like a good way to learn how the XDR parts of Player work. I'm using the > method Brian suggested on the task manager page for it. So far I haven't > done much, just got the playerxdrgen.py script to recognise pointers and > find the _count variable. > > The main implementation issue, and the one that's always killed > discussion in the past, seems to be where memory is > allocated/deallocated. My understanding of the way the XDR stuff works > is that a buffer is allocated for the packed data just before it is > packed and sent over the transport. It's easy enough to allocate this > buffer correctly for the size of a variable length array. So the issue > is driver and client writers needing to allocate/deallocate. > > Drivers and clients would probably have to allocate the array themselves > before publishing, sending requests, etc, and this is where memory leak > issues start to appear. I can imagine that most drivers using an > interface with a variable length array would be sending regular large > chunks of data of a similar size (such as a video frame) and in this > case could reuse the array for each new message they publish. Same for > clients making regular requests. Otherwise they'll have to allocate and > remember to deallocate the array in their local copy of the message > themselves. > > Because drivers/clients may want to reuse a message structure, including > any variable length arrays, after publishing a message, I don't think we > can put deallocation of the array into the XDR packing function so that > it gets deleted when the message is packed. But we could possibly add a > check for if the array pointer is NULL for unpacking, and have the > function allocate necessary memory in this case. Although in the case of > clients, the buffer is already allocated to be max message size so no > need to allocate anything there, and if the buffer to unpack into isn't > zeroed the packing function may not allocate as necessary. Since > structures lack intelligence we can't add constructors/destructors > without forcing the entire interface system into C++, which probably > isn't desirable. > > That's about as far as I've got in thinking about how to do this so far. > Does anyone else have any comments? > > Geoff -- Robotics research group, University of Auckland http://www.ece.auckland.ac.nz/~gbig005/ |
From: Radu B. R. <ru...@cs...> - 2007-02-04 20:09:18
|
Geoffrey Biggs wrote: > - Dynamic arrays cannot be in embedded structures contained within a > message. For example, if you have an array of structures as part of the > message, those structures cannot contain dynamically allocated arrays. > This is because of the way memory allocation is handled for unpacking > received messages. The xdr filter function called for the embedded > structure type doesn't know if it's encoding or decoding, and so doesn't > know if it needs to allocate memory or not. I'm not sure how to get > around this problem yet. Would this be possible if additional information were specified in the message? Or, what about changing the packing function by generating two signatures: one for XDR_ENCODE and one for XDR_DECODE ? Not sure how or if this would work though yet. > - When an array is encoded with XDR, it seems to encode the pointer into > the message as well, even though this is rather useless. It results in 4 > extra bytes being sent. I'm not sure why this is happening, perhaps > someone more familiar with XDR knows. I remember running into this last year while building Javaclient2. I searched the archives and found Brian's explanation: http://www.nabble.com/player2-thoughts-tf1123613.html#a2939874. > A. When a message goes from client to driver: > 1 - Client allocates local copy (handled by client writer). > 2 - Client deletes local copy (because client is single-threaded for > writing to server, no conflict with needing to wait till message gets > packed). > 3 - Unpacking function on server allocates (handled automatically). > 4 - Somewhere on the server, once message has been handled, array needs > to be deleted (handled by driver writer?). [...] > A.4 shouldn't be too bad, but it's not that nice for drivers to have to > free memory allocated elsewhere once they've handled a message. I would solve A.4 by building some driverbase classes (the same way we have imagebase now for example) and make the actual drivers inherit from it/them, or, put additional constraints on the way the driver code should look. The latter would be also useful for easier future modifications of a driver by someone else as well as for keeping a certain structure in place. Note that right now different drivers look very different, implementation-wise, from each other. This is ok for 3rd party drivers which are not in the repository, but for those included in the Player distro, it makes things a bit tougher to manage over time. (One trivial example where I saw something like this useful was to have different debug levels for each driver in particular - different than the ones for the server - which range from some simple messages like "port open", "port closed" to higher levels like actually displaying the packages/bytes being sent over the wire to the hardware device on screen). > B. When a message goes from driver to client: > 1 - Driver allocates local copy (handled by driver writer) > 2 - Memory allocated by driver needs to be deleted after message is > packed. Due to threading, message may not be packed as soon as Publish > is called, so cannot delete after Publish returns (memory leak!). > 3 - Unpacking function on client allocates (handled automatically). > 4 - Client library deletes after handling message, or passes ownership > on via returning to client program (handled either by client lib dev or > client program dev). > > The problematic ones are the deletes: > > B.4 could be problematic if other transport layers are introduces which > handle dynamic arrays better and so don't need the client library to > delete manually. > > B.2 is the worst. Because of threading issues, the driver can't free the > memory itself without risking a race condition, nor can it reuse that > memory. The playertcp can't free it because it doesn't know how to (a > message body is opaque to it, and, as far as it knows, static). What > should probably happen here is that when Publish is called a deep copy > of the message structure is made instead of a shallow copy (which works > fine so far because we haven't had dynamically allocated memory to worry > about), but because the inside of messages isn't known, this is difficult. I think B.4 is not a big problem, because if a different transport mechanism will be implemented (by making use of ace, ice, corba, etc or similar), it usually have its own packing structures, so I think we wouldn't end up using XDR for it anyway. Somebody please correct me if I'm wrong. I agree with B.2, and at first glance, I also see deep copying as the immediate solution. How much overhead would that add though? > One possible way to manage all this deleting is created a > FinishMessage() function or similar that, when called, will clean up a > message after it's been used (either packed or handled). This function > will need to know how to clean up each message, so a customised function > for each message is necessary. However, rather than writing these, we > could automatically generate them similar to the packing functions, and > use a function table to look them up in FinishMessage() and call the > appropriate one. Messages without any dynamically allocated memory would > have an empty function, and if we inlined these they would disappear > (optimising compilers should remove empty functions anyway). The > function table lookup could be considered wasteful, though, when the > bulk of messages don't need cleaning up. Auto-generation could also be > used to create functions to handle deep copies of messages made when > they get pushed onto message queues. This sounds cool. What are the drawbacks ? I didn't understand the last part where you said that "when the bulk of messages don't need cleaning up". Anyway, very very cool analysis Geoff! I hope others will join the discussion so we can move towards a ready-to-implement phase. Cheers, Radu. -- | Radu Bogdan Rusu | http://rbrusu.com/ | http://www9.cs.tum.edu/people/rusu/ | Intelligent Autonomous Systems | Technische Universitaet Muenchen |
From: Geoffrey B. <g....@au...> - 2007-02-04 22:42:07
|
Radu Bogdan Rusu wrote: > Would this be possible if additional information were specified in the message? Or, what > about changing the packing function by generating two signatures: one for XDR_ENCODE and > one for XDR_DECODE ? Not sure how or if this would work though yet. The issue is the XDR filter functions, not the packing functions as such. The packing functions receive an argument specifying if they are encoding or decoding, but this is not passed to filter functions used to pack structures within structures. As far as I can tell these filter functions must comply to a specific function definition specified by XDR. > I remember running into this last year while building Javaclient2. I searched the archives > and found Brian's explanation: http://www.nabble.com/player2-thoughts-tf1123613.html#a2939874. In that case it's not a problem. > I would solve A.4 by building some driverbase classes (the same way we have imagebase now > for example) and make the actual drivers inherit from it/them, or, put additional > constraints on the way the driver code should look. The latter would be also useful for > easier future modifications of a driver by someone else as well as for keeping a certain > structure in place. Note that right now different drivers look very different, > implementation-wise, from each other. This is ok for 3rd party drivers which are not in > the repository, but for those included in the Player distro, it makes things a bit tougher > to manage over time. (One trivial example where I saw something like this useful was to > have different debug levels for each driver in particular - different than the ones for > the server - which range from some simple messages like "port open", "port closed" to > higher levels like actually displaying the packages/bytes being sent over the wire to the > hardware device on screen). While driverbase classes would be nice, at least in the short term a better way would, I think be to use the FinishMessage() function method and call it after the message dispatch process is completed. This means that the part of player that created the message (the core) also deletes its dynamic parts. As an aside, I can agree on the debug levels thing. I recently added them to the ALSA driver after finding that the existing Player debug message system was not suitable. > I think B.4 is not a big problem, because if a different transport mechanism will be > implemented (by making use of ace, ice, corba, etc or similar), it usually have its own > packing structures, so I think we wouldn't end up using XDR for it anyway. Somebody please > correct me if I'm wrong. The issue with it arises specifically when other transports are used which do not utilise XDR and may take care of their own dynamically allocated memory. The client library would then be deleting memory it shouldn't. > I agree with B.2, and at first glance, I also see deep copying as the immediate solution. > How much overhead would that add though? The overhead would be in looking up the copy function. Other than that, it's not copying any more data than if the data were statically allocated in memory. >> One possible way to manage all this deleting is created a >> FinishMessage() function or similar that, when called, will clean up a >> message after it's been used (either packed or handled). This function >> will need to know how to clean up each message, so a customised function >> for each message is necessary. However, rather than writing these, we >> could automatically generate them similar to the packing functions, and >> use a function table to look them up in FinishMessage() and call the >> appropriate one. Messages without any dynamically allocated memory would >> have an empty function, and if we inlined these they would disappear >> (optimising compilers should remove empty functions anyway). The >> function table lookup could be considered wasteful, though, when the >> bulk of messages don't need cleaning up. Auto-generation could also be >> used to create functions to handle deep copies of messages made when >> they get pushed onto message queues. > > This sounds cool. What are the drawbacks ? I didn't understand the last part where you > said that "when the bulk of messages don't need cleaning up". The drawback is the need to perform a function lookup to clean up a message. By "when the bulk of messages don't need cleaning up," I mean that most messages are purely statically allocated structures, without dynamically allocated arrays. They can be removed from memory easily. It's the ones with dynamically allocated arrays that need special functions. If we have to look up a function to delete a message, we are looking at some inefficiency for those messages which could be cleaned up without a customised function. Geoff -- Robotics research group, University of Auckland http://www.ece.auckland.ac.nz/~gbig005/ |
From: Radu B. R. <ru...@cs...> - 2007-02-07 13:07:22
|
Geoffrey Biggs wrote: > Radu Bogdan Rusu wrote: >> I think B.4 is not a big problem, because if a different transport mechanism will be >> implemented (by making use of ace, ice, corba, etc or similar), it usually have its own >> packing structures, so I think we wouldn't end up using XDR for it anyway. Somebody please >> correct me if I'm wrong. > > The issue with it arises specifically when other transports are used > which do not utilise XDR and may take care of their own dynamically > allocated memory. The client library would then be deleting memory it > shouldn't. I believe that the client library should implement checks to see what protocol/transport mechanism it uses, so that situation will be bypassed. Of course, the biggest disadvantage on using "3rd party communication frameworks" is that we lose a big chunk of the Player "client libraries", meaning that no one will ever want to implement wrappers or code a LISP compatible version for ICE or ACE. :) That is why the only client libraries who might work straightforwardly with any additional transport mechanism would be the C/C++ (perhaps any of the bindings could also work? although I don't see this happening too easily) and probably the Java ones. So, not too many things to change there I guess. >> I agree with B.2, and at first glance, I also see deep copying as the immediate solution. >> How much overhead would that add though? > > The overhead would be in looking up the copy function. Other than that, > it's not copying any more data than if the data were statically > allocated in memory. That is nothing I think. An acceptable penalty. > The drawback is the need to perform a function lookup to clean up a > message. By "when the bulk of messages don't need cleaning up," I mean > that most messages are purely statically allocated structures, without > dynamically allocated arrays. They can be removed from memory easily. > It's the ones with dynamically allocated arrays that need special > functions. If we have to look up a function to delete a message, we are > looking at some inefficiency for those messages which could be cleaned > up without a customised function. Ah, I see. Maybe this inefficiency will be less than what we think it is right now, after we implement it. Having lookup tables and searching through them fast is something pretty standard these days and can be optimized easily. > Geoff So how would a "dynamic structure" (couldn't coin up a better term right now) look like then ? Let's exemplify on the camera data packet which is one of the biggest ones right now. Cheers, Radu. -- | Radu Bogdan Rusu | http://rbrusu.com/ | http://www9.cs.tum.edu/people/rusu/ | Intelligent Autonomous Systems | Technische Universitaet Muenchen |
From: Brian G. <br...@ge...> - 2007-02-27 18:14:58
|
On Feb 7, 2007, at 9:16 PM, Geoffrey Biggs wrote: > I've gone ahead and implemented the necessary stuff to do all this. I > submitted it as a patch so we can keep track of it. As we fine tune it > we can update the patch until we have something ready to commit to > CVS. Thanks very much for all the work, Geoff. I've applied Geoff's patch on HEAD. As of now, it's not really being used, because there aren't any dynamic-length arrays in player.h. So from here we can start making the bigger arrays into dynamic structures and see how it goes. Patches happily accepted. This is a big change, so I tagged the state of CVS prior to committing this patch with 'before-dynarray-patch'. brian. |
From: Geoffrey B. <g....@au...> - 2007-03-01 05:02:01
|
I've submitted a patch that moves the audio interface to a dynamic array for wave data. This is a relatively simple dynamic array, so it provides a good example of what needs to be done to change an interface to using dynamic data. Those interested in altering other interfaces should take a look. http://sourceforge.net/tracker/index.php?func=detail&aid=1671417&group_id=42445&atid=433166 Geoff Brian Gerkey wrote: > I've applied Geoff's patch on HEAD. As of now, it's not really being > used, because there aren't any dynamic-length arrays in player.h. So > from here we can start making the bigger arrays into dynamic > structures and see how it goes. Patches happily accepted. -- Robotics research group, University of Auckland http://www.ece.auckland.ac.nz/~gbig005/ |
From: Geoffrey B. <g....@au...> - 2007-02-08 05:16:14
|
I've gone ahead and implemented the necessary stuff to do all this. I submitted it as a patch so we can keep track of it. As we fine tune it we can update the patch until we have something ready to commit to CVS. http://sourceforge.net/tracker/index.php?func=detail&aid=1654805&group_id=42445&atid=433166 Please keep in mind that this is very much a work in progress. I still don't know if I've caught all the allocation situations. What it does currently is: - Generates _dpcpy and _cleanup functions for those structures with dynamically allocated data. Function table entries must have an extra two pointers on the end, NULL for most structures but filled in for those structures with dynamic data. - Adds a call in the C client library to the clean up function for messages. This call is placed immediately after the dispatch method returns, meaning that dynamic data created during unpacking will be cleaned up after the client library has used it. Client library functions (those in the dev_*.c files) need to be sure they copy any data from dynamic stuff, not just pointers. This solves B.4. - Adds a call in playertcp and playerudp to clean up data allocated during unpacking. - Makes the Message class conduct a deep copy where necessary of message data rather than just a shallow copy of the structure. Similarly, when Message destructs, it deletes dynamically allocated data. This ensures that whenever messages move around Player (as they do outside of drivers) dynamic data is copied where necessary and deleted when finished with. Thanks to the reference counting system in Message objects, the deep copying should be minimised. This solves A.4 and B.2. With this, all the allocation/deallocation issues I'm aware of should be solved. You should be able to see how I've handled the C client library and mimic that in the Java one without too much trouble. There is still an outstanding issue from last time. Copying my previous message: - Dynamic arrays cannot be in embedded structures contained within a message. For example, if you have an array of structures as part of the message, those structures cannot contain dynamically allocated arrays. This is because of the way memory allocation is handled for unpacking received messages. The xdr filter function called for the embedded structure type doesn't know if it's encoding or decoding, and so doesn't know if it needs to allocate memory or not. I'm not sure how to get around this problem yet. More comments interspersed below. Radu Bogdan Rusu wrote: > I believe that the client library should implement checks to see what protocol/transport > mechanism it uses, so that situation will be bypassed. Of course, the biggest disadvantage > on using "3rd party communication frameworks" is that we lose a big chunk of the Player > "client libraries", meaning that no one will ever want to implement wrappers or code a > LISP compatible version for ICE or ACE. :) > That is why the only client libraries who might work straightforwardly with any additional > transport mechanism would be the C/C++ (perhaps any of the bindings could also work? > although I don't see this happening too easily) and probably the Java ones. So, not too > many things to change there I guess. Ideally, the client libraries would be independent of transport used. The patch I've submitted at least keeps all the stuff dealing with XDR data allocation in client.c and out of the dev_*.c files. > Ah, I see. Maybe this inefficiency will be less than what we think it is right now, after > we implement it. Having lookup tables and searching through them fast is something pretty > standard these days and can be optimized easily. We should probably test how much of an impact the patch I've submitted has on performance. > So how would a "dynamic structure" (couldn't coin up a better term right now) look like > then ? Let's exemplify on the camera data packet which is one of the biggest ones right now. I've been using the audio interface to test with, as I recently made some significant changes to the ALSA driver which mean it would benefit from dynamic arrays in the interface (those changes aren't committed yet as I need to get some other interface changes committed to player.h for me first, so don't go looking :)), plus it's the most convenient device I have handy right now. The specific messages I used were recorded wave data (server->client) and playing a wave (client->server). The structure used for both looks like this with static allocation: typedef struct player_audio_wav { /** length of raw data */ uint32_t data_count; /** raw data */ uint8_t *data; /** Raw data format */ uint32_t format; } player_audio_wav_t; It looks like this with dynamic allocation: typedef struct player_audio_wav { /** length of raw data */ uint32_t data_count; /** raw data */ uint8_t *data; /** Raw data format */ uint32_t format; } player_audio_wav_t; So not a huge change in structure layout, since the _count variable is still necessary for XDR and message receivers to know how much data they're getting. I didn't include these changes in the patch, by the way. The camera interface could be modified to look like this: typedef struct player_camera_data { /** Image dimensions [pixels]. */ uint32_t width; /** Image dimensions [pixels]. */ uint32_t height; /** Image bits-per-pixel (8, 16, 24, 32). */ uint32_t bpp; /** Image format (must be compatible with depth). */ uint32_t format; /** Some images (such as disparity maps) use scaled pixel values; for these images, fdiv specifies the scale divisor (i.e., divide the integer pixel value by fdiv to recover the real pixel value). */ uint32_t fdiv; /** Image compression; @ref PLAYER_CAMERA_COMPRESS_RAW indicates no compression. */ uint32_t compression; /** Size of image data as stored in image buffer (bytes) */ uint32_t image_count; /** Compressed image data (byte-aligned, row major order). Multi-byte image formats (such as MONO16) must be converted to network byte ordering. */ uint8_t image*; } player_camera_data_t; Geoff -- Robotics research group, University of Auckland http://www.ece.auckland.ac.nz/~gbig005/ |
From: Geoffrey B. <g....@au...> - 2007-02-09 23:02:10
|
I've uploaded a new version of the patch. I discovered that I had missed one issue with the previous one: the transport libraries were allocating an XDR buffer based on the size reported in the message header, which didn't take into account any dynamically allocated data. I missed this because I had been beefing up the message header size in calls to Publish() to compensate for this in my first prototype, and forgot to remove this when I added the deep copy stuff. This leads to the Message class allocating and copying beyond the size of the static message structure, which is clearly a memory access violation (although since it's only a read, it doesn't cause an error most of the time). The fix was to make the deep copy function return the number of bytes copied and store this value in the Message class. The transport libraries now allocate an XDR buffer based on both the static message component size and any dynamic data present, meaning there will be enough for all parts of the message. Calls to Publish() only need to specify the size of the message structure. However, this may be confusing for programmers, so a neater fix could be to add an argument to Publish() which specifies the size of dynamic data in the message. Geoff -- Robotics research group, University of Auckland http://www.ece.auckland.ac.nz/~gbig005/ |
From: Geoffrey B. <g....@au...> - 2007-02-11 04:04:56
|
New version of the patch up. This one is, I hope, getting fairly close to complete. It solves the issue of embedded structures, meaning dynamic data can be used anywhere in a message now. I don't know of any other issues with the new stuff added. All this needs now is testing by a few people, and hopefully we can close off one of the most common complaints with Player. Geoff -- Robotics research group, University of Auckland http://www.ece.auckland.ac.nz/~gbig005/ |