Re: [pulseaudio-discuss] [PATCH] Add a target to the PA log feature and improve PA log core
From: Maarten Bosmans [mailto:mkbosm...@gmail.com] 2011/2/23 Becker, VincentX vincentx.bec...@intel.com: Thanks for your review Maarten. So you suggest to split this into several patches (2). One for the outer implementation and the other for the inner one. I will try to attend to the irc meeting tomorrow so I can catch your remarks in real time. I'm not sure what you mean by outer and inner implementation. What I meant was one patch thats adds the option to log to a file (this patch touches all four files) and one patch that does the metadata, append_data thing (should only change src/pulsecore/log.c) Having two clean patches (see my other comments) tomorrow would help to keep thing moving in the meeting. Hi Maarten, Here are the 2 patches as you suggested (both patches can be compiled separately). I also integrated one of your remarks, but not all. Like concerning the file naming, as you said it might be a bit too complicated and appending the file or creating a new one might be enough. But it remains a powerful way to debug pulseaudio and it is more direct than using syslog. I also wrote a module dedicated to log PCM samples, configurable for sinks and/or sources with or without their respective sink inputs/source outputs. I will submit it probably next month. Vince Sorry for getting this rolling on such a short notice. No problem at all! Maarten Cheers, V. - Intel Corporation SAS (French simplified joint stock company) Registered headquarters: Les Montalets- 2, rue de Paris, 92196 Meudon Cedex, France Registration Number: 302 456 199 R.C.S. NANTERRE Capital: 4,572,000 Euros This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. 0001-Add-a-new-log-target-to-a-file-descriptor-in-daemon-.patch Description: 0001-Add-a-new-log-target-to-a-file-descriptor-in-daemon-.patch 0001-Format-log-messages-with-generic-prepended-and-appen.patch Description: 0001-Format-log-messages-with-generic-prepended-and-appen.patch ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] Add a target to the PA log feature and improve PA log core
2011/2/24 Becker, VincentX vincentx.bec...@intel.com: From: Maarten Bosmans [mailto:mkbosm...@gmail.com] 2011/2/23 Becker, VincentX vincentx.bec...@intel.com: Thanks for your review Maarten. So you suggest to split this into several patches (2). One for the outer implementation and the other for the inner one. I will try to attend to the irc meeting tomorrow so I can catch your remarks in real time. I'm not sure what you mean by outer and inner implementation. What I meant was one patch thats adds the option to log to a file (this patch touches all four files) and one patch that does the metadata, append_data thing (should only change src/pulsecore/log.c) Having two clean patches (see my other comments) tomorrow would help to keep thing moving in the meeting. Hi Maarten, Here are the 2 patches as you suggested (both patches can be compiled separately). I also integrated one of your remarks, but not all. Like concerning the file naming, as you said it might be a bit too complicated and appending the file or creating a new one might be enough. But it remains a powerful way to debug pulseaudio and it is more direct than using syslog. I also wrote a module dedicated to log PCM samples, configurable for sinks and/or sources with or without their respective sink inputs/source outputs. I will submit it probably next month. Vince But these two patches cannot be appied on top of eachother, as PA_LOG_FILED is added in log.h in both. Also, I would expect the two case PA_LOG_FILED branches to be introduced in the 0001-Add-a-new-log-target-to-a-file-descriptor-in-daemon patch, not the other one. Anyway, I added a link to the meeting page http://pulseaudio.org/wiki/Meetings/2011-02-24 , we'll see where it goes. Maarten ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] Add a target to the PA log feature and improve PA log core
Here's my review of that patch. It will be handled in the meeting tomorrow, but I'm sure I've forgotten by then, so I sent it to the list. 2010/11/22 Becker, VincentX vincentx.bec...@intel.com: Hi all, Could you please review this patch ? This is a patch that adds a generic log target for PA log (log.c). This new target can be either a char device or a regular file. For our remote log device needs, it has been necessary to directly log Pulseaudio traces to a file descriptor. Also optimizations in the memory allocations have been done to reuse local pointers to prevent extra memory allocations (on stack or heap), that would happen in case of very big trace buffers, and that could potentially have an impact on audio latency in embedded systems. Modifications : src/daemon/cmdline.c : Update Pulseaudio help and error message for the 'log-target' command argument. The current help message has a width of 80 characters. It would be nice to keep it that way and find another way to fit the new log-target argument in there. src/daemon/daemon-conf.c : Add the condition when the target given is a file name given with relative or absolute path. If the file already exists, it creates a new file that will contain the current date and time inserted in the name. For this purpose, two static utilities functions have been added. The file descriptor is then passed to the PA log in pulsecore. I don't think the renaming with the data appended to the filename is functionality that belongs in pulseaudio. If you want serious logging, use syslog, if you want quick-and-dirty, use the filename. Pulse should just append or overwrite the file, either is fine. Also, as daemon-conf opens the file, it should call pa_close itself too. Then the pa_log_close_fd function can be removed. src/pulsecore/log.c : The function pa_log_levelv_meta formats the text data and sends it to the appropriate target. There are 2 main changes : 1) Add the target PA_LOG_FILED which allows to write the formatted log data to a file descriptor output. Add functions to open and close that file descriptor. This would better be called PA_LOG_FD 2) the algorithms around 'char *t' have been reordered in order to optimize memory use. This could be useful when traces are big. The trace buffer char text has been set to 16*1024Kb which allocates already 16Kb on the stack. This buffer is then copied into t, in the for loop that checks for carriage returns. What needed to be optimized is that extra memory is allocated in case of metadata (location and timestamp) is prepended to t, by creating dynamically a new buffer. The idea is to prepend the data directly into t (and append if it's the case) before we affect the value of 'text'. It avoids one dynamic memory allocation, at least in the case of the new target PA_LOG_FILED. Therefore, a 'metadata' buffer is created and prepended in t whatever the target. One switch/case is actually added to build this metadata buffer and we keep the other one just for write the actual log (text+metadata) in the target. Please separate this change out in another patch. That way it can be reviewed better. P.S. : there are bad indentations due to tabs. I use emacs and tried the indentation Lisp function given here but without success : http://www.pulseaudio.org/wiki/CodingStyle . It keeps putting tabs instead of spaces ! How could I solve that ? Thanks, Vincent Maarten Vincent BECKER Intel - Wireless System Integration Engineer Medfield platform - Intel Corporation SAS (French simplified joint stock company) Registered headquarters: Les Montalets- 2, rue de Paris, 92196 Meudon Cedex, France Registration Number: 302 456 199 R.C.S. NANTERRE Capital: 4,572,000 Euros This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. Ah, to bad you send it to a public list then. I guess we're all criminals now ;-) ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] Add a target to the PA log feature and improve PA log core
Here's my review of that patch. It will be handled in the meeting tomorrow, but I'm sure I've forgotten by then, so I sent it to the list. 2010/11/22 Becker, VincentX vincentx.bec...@intel.com: Hi all, Could you please review this patch ? This is a patch that adds a generic log target for PA log (log.c). This new target can be either a char device or a regular file. For our remote log device needs, it has been necessary to directly log Pulseaudio traces to a file descriptor. Also optimizations in the memory allocations have been done to reuse local pointers to prevent extra memory allocations (on stack or heap), that would happen in case of very big trace buffers, and that could potentially have an impact on audio latency in embedded systems. Modifications : src/daemon/cmdline.c : Update Pulseaudio help and error message for the 'log-target' command argument. The current help message has a width of 80 characters. It would be nice to keep it that way and find another way to fit the new log-target argument in there. src/daemon/daemon-conf.c : Add the condition when the target given is a file name given with relative or absolute path. If the file already exists, it creates a new file that will contain the current date and time inserted in the name. For this purpose, two static utilities functions have been added. The file descriptor is then passed to the PA log in pulsecore. I don't think the renaming with the data appended to the filename is functionality that belongs in pulseaudio. If you want serious logging, use syslog, if you want quick-and-dirty, use the filename. Pulse should just append or overwrite the file, either is fine. Also, as daemon-conf opens the file, it should call pa_close itself too. Then the pa_log_close_fd function can be removed. src/pulsecore/log.c : The function pa_log_levelv_meta formats the text data and sends it to the appropriate target. There are 2 main changes : 1) Add the target PA_LOG_FILED which allows to write the formatted log data to a file descriptor output. Add functions to open and close that file descriptor. This would better be called PA_LOG_FD 2) the algorithms around 'char *t' have been reordered in order to optimize memory use. This could be useful when traces are big. The trace buffer char text has been set to 16*1024Kb which allocates already 16Kb on the stack. This buffer is then copied into t, in the for loop that checks for carriage returns. What needed to be optimized is that extra memory is allocated in case of metadata (location and timestamp) is prepended to t, by creating dynamically a new buffer. The idea is to prepend the data directly into t (and append if it's the case) before we affect the value of 'text'. It avoids one dynamic memory allocation, at least in the case of the new target PA_LOG_FILED. Therefore, a 'metadata' buffer is created and prepended in t whatever the target. One switch/case is actually added to build this metadata buffer and we keep the other one just for write the actual log (text+metadata) in the target. Please separate this change out in another patch. That way it can be reviewed better. P.S. : there are bad indentations due to tabs. I use emacs and tried the indentation Lisp function given here but without success : http://www.pulseaudio.org/wiki/CodingStyle . It keeps putting tabs instead of spaces ! How could I solve that ? Thanks, Vincent Maarten Vincent BECKER Intel - Wireless System Integration Engineer Medfield platform - Intel Corporation SAS (French simplified joint stock company) Registered headquarters: Les Montalets- 2, rue de Paris, 92196 Meudon Cedex, France Registration Number: 302 456 199 R.C.S. NANTERRE Capital: 4,572,000 Euros This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. Ah, to bad you send it to a public list then. I guess we're all criminals now ;-) ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] Add a target to the PA log feature and improve PA log core
-Original Message- From: Maarten Bosmans [mailto:mkbosm...@gmail.com] Sent: Wednesday, February 23, 2011 3:50 PM To: General PulseAudio Discussion Cc: Becker, VincentX Subject: Re: [pulseaudio-discuss] [PATCH] Add a target to the PA log feature and improve PA log core Here's my review of that patch. It will be handled in the meeting tomorrow, but I'm sure I've forgotten by then, so I sent it to the list. 2010/11/22 Becker, VincentX vincentx.bec...@intel.com: Hi all, Could you please review this patch ? This is a patch that adds a generic log target for PA log (log.c). This new target can be either a char device or a regular file. For our remote log device needs, it has been necessary to directly log Pulseaudio traces to a file descriptor. Also optimizations in the memory allocations have been done to reuse local pointers to prevent extra memory allocations (on stack or heap), that would happen in case of very big trace buffers, and that could potentially have an impact on audio latency in embedded systems. Modifications : src/daemon/cmdline.c : Update Pulseaudio help and error message for the 'log-target' command argument. The current help message has a width of 80 characters. It would be nice to keep it that way and find another way to fit the new log-target argument in there. src/daemon/daemon-conf.c : Add the condition when the target given is a file name given with relative or absolute path. If the file already exists, it creates a new file that will contain the current date and time inserted in the name. For this purpose, two static utilities functions have been added. The file descriptor is then passed to the PA log in pulsecore. I don't think the renaming with the data appended to the filename is functionality that belongs in pulseaudio. If you want serious logging, use syslog, if you want quick-and-dirty, use the filename. Pulse should just append or overwrite the file, either is fine. Also, as daemon-conf opens the file, it should call pa_close itself too. Then the pa_log_close_fd function can be removed. src/pulsecore/log.c : The function pa_log_levelv_meta formats the text data and sends it to the appropriate target. There are 2 main changes : 1) Add the target PA_LOG_FILED which allows to write the formatted log data to a file descriptor output. Add functions to open and close that file descriptor. This would better be called PA_LOG_FD 2) the algorithms around 'char *t' have been reordered in order to optimize memory use. This could be useful when traces are big. The trace buffer char text has been set to 16*1024Kb which allocates already 16Kb on the stack. This buffer is then copied into t, in the for loop that checks for carriage returns. What needed to be optimized is that extra memory is allocated in case of metadata (location and timestamp) is prepended to t, by creating dynamically a new buffer. The idea is to prepend the data directly into t (and append if it's the case) before we affect the value of 'text'. It avoids one dynamic memory allocation, at least in the case of the new target PA_LOG_FILED. Therefore, a 'metadata' buffer is created and prepended in t whatever the target. One switch/case is actually added to build this metadata buffer and we keep the other one just for write the actual log (text+metadata) in the target. Please separate this change out in another patch. That way it can be reviewed better. P.S. : there are bad indentations due to tabs. I use emacs and tried the indentation Lisp function given here but without success : http://www.pulseaudio.org/wiki/CodingStyle . It keeps putting tabs instead of spaces ! How could I solve that ? Thanks, Vincent Maarten Thanks for your review Maarten. So you suggest to split this into several patches (2). One for the outer implementation and the other for the inner one. I will try to attend to the irc meeting tomorrow so I can catch your remarks in real time. Cheers, V. Vincent BECKER Intel - Wireless System Integration Engineer Medfield platform Ah, to bad you send it to a public list then. I guess we're all criminals now ;-) :-) ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss - Intel Corporation SAS (French simplified joint stock company) Registered headquarters: Les Montalets- 2, rue de Paris, 92196 Meudon Cedex, France Registration Number: 302 456 199 R.C.S. NANTERRE Capital: 4,572,000 Euros This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de
Re: [pulseaudio-discuss] [PATCH] Add a target to the PA log feature and improve PA log core
2011/2/23 Becker, VincentX vincentx.bec...@intel.com: Thanks for your review Maarten. So you suggest to split this into several patches (2). One for the outer implementation and the other for the inner one. I will try to attend to the irc meeting tomorrow so I can catch your remarks in real time. I'm not sure what you mean by outer and inner implementation. What I meant was one patch thats adds the option to log to a file (this patch touches all four files) and one patch that does the metadata, append_data thing (should only change src/pulsecore/log.c) Having two clean patches (see my other comments) tomorrow would help to keep thing moving in the meeting. Sorry for getting this rolling on such a short notice. Maarten Cheers, V. ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
[pulseaudio-discuss] [PATCH] Add a target to the PA log feature and improve PA log core
Hi all, Could you please review this patch ? This is a patch that adds a generic log target for PA log (log.c). This new target can be either a char device or a regular file. For our remote log device needs, it has been necessary to directly log Pulseaudio traces to a file descriptor. Also optimizations in the memory allocations have been done to reuse local pointers to prevent extra memory allocations (on stack or heap), that would happen in case of very big trace buffers, and that could potentially have an impact on audio latency in embedded systems. Modifications : src/daemon/cmdline.c : Update Pulseaudio help and error message for the 'log-target' command argument. src/daemon/daemon-conf.c : Add the condition when the target given is a file name given with relative or absolute path. If the file already exists, it creates a new file that will contain the current date and time inserted in the name. For this purpose, two static utilities functions have been added. The file descriptor is then passed to the PA log in pulsecore. src/pulsecore/log.c : The function pa_log_levelv_meta formats the text data and sends it to the appropriate target. There are 2 main changes : 1) Add the target PA_LOG_FILED which allows to write the formatted log data to a file descriptor output. Add functions to open and close that file descriptor. 2) the algorithms around 'char *t' have been reordered in order to optimize memory use. This could be useful when traces are big. The trace buffer char text has been set to 16*1024Kb which allocates already 16Kb on the stack. This buffer is then copied into t, in the for loop that checks for carriage returns. What needed to be optimized is that extra memory is allocated in case of metadata (location and timestamp) is prepended to t, by creating dynamically a new buffer. The idea is to prepend the data directly into t (and append if it's the case) before we affect the value of 'text'. It avoids one dynamic memory allocation, at least in the case of the new target PA_LOG_FILED. Therefore, a 'metadata' buffer is created and prepended in t whatever the target. One switch/case is actually added to build this metadata buffer and we keep the other one just for write the actual log (text+metadata) in the target. P.S. : there are bad indentations due to tabs. I use emacs and tried the indentation Lisp function given here but without success : http://www.pulseaudio.org/wiki/CodingStyle . It keeps putting tabs instead of spaces ! How could I solve that ? Thanks, Vincent Vincent BECKER Intel - Wireless System Integration Engineer Medfield platform - Intel Corporation SAS (French simplified joint stock company) Registered headquarters: Les Montalets- 2, rue de Paris, 92196 Meudon Cedex, France Registration Number: 302 456 199 R.C.S. NANTERRE Capital: 4,572,000 Euros This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. 0001-Pulseaudio-Add-a-target-to-PA-log-feature-and-optimi.patch Description: 0001-Pulseaudio-Add-a-target-to-PA-log-feature-and-optimi.patch ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
