[patch 0/5] A few simple MPlayer patches from Google
Hi, we at Google have a few simple bugfixes we use internally and we would like to upstream them: - Increase bitrate limits for lavc encoding - Don't hardcode path length in libmpdemux/mf.c - stream_write_buffer return value is never checked, make the function retry by itself - add aqscale lavcopt (equivalent to ffmpeg -aq) - fix libmpdemux/muxer.c memleak Cheers, Tobias
This about quadruples the maximum from 24Mbit to to 100Mbit. Maybe use INT_MAX or something to not limit the user in any way? Original patch by Krishna Gadepalli and Debargha Mukherjee. Signed-off-by: Tobias Diedrich <ranma@google.com> Index: mplayer-patchset1/libmpcodecs/ve_lavc.c =================================================================== --- mplayer-patchset1.orig/libmpcodecs/ve_lavc.c 2010-12-21 19:48:53.897993000 +0100 +++ mplayer-patchset1/libmpcodecs/ve_lavc.c 2010-12-21 19:51:07.721412000 +0100 @@ -183,13 +183,15 @@ #include "m_option.h" +#define MAX_BITRATE 100000000 /* 100Mbit */ + const m_option_t lavcopts_conf[]={ {"acodec", &lavc_param_acodec, CONF_TYPE_STRING, 0, 0, 0, NULL}, {"abitrate", &lavc_param_abitrate, CONF_TYPE_INT, CONF_RANGE, 1, 1000000, NULL}, {"atag", &lavc_param_atag, CONF_TYPE_INT, CONF_RANGE, 0, 0xffff, NULL}, {"vcodec", &lavc_param_vcodec, CONF_TYPE_STRING, 0, 0, 0, NULL}, - {"vbitrate", &lavc_param_vbitrate, CONF_TYPE_INT, CONF_RANGE, 4, 24000000, NULL}, - {"vratetol", &lavc_param_vrate_tolerance, CONF_TYPE_INT, CONF_RANGE, 4, 24000000, NULL}, + {"vbitrate", &lavc_param_vbitrate, CONF_TYPE_INT, CONF_RANGE, 4, MAX_BITRATE, NULL}, + {"vratetol", &lavc_param_vrate_tolerance, CONF_TYPE_INT, CONF_RANGE, 4, MAX_BITRATE, NULL}, {"vhq", &lavc_param_mb_decision, CONF_TYPE_FLAG, 0, 0, 1, NULL}, {"mbd", &lavc_param_mb_decision, CONF_TYPE_INT, CONF_RANGE, 0, 9, NULL}, {"v4mv", &lavc_param_v4mv, CONF_TYPE_FLAG, 0, 0, 1, NULL}, @@ -227,9 +229,9 @@ {"vqmod_freq", &lavc_param_rc_qmod_freq, CONF_TYPE_INT, 0, 0, 0, NULL}, {"vrc_eq", &lavc_param_rc_eq, CONF_TYPE_STRING, 0, 0, 0, NULL}, {"vrc_override", &lavc_param_rc_override_string, CONF_TYPE_STRING, 0, 0, 0, NULL}, - {"vrc_maxrate", &lavc_param_rc_max_rate, CONF_TYPE_INT, CONF_RANGE, 0, 24000000, NULL}, - {"vrc_minrate", &lavc_param_rc_min_rate, CONF_TYPE_INT, CONF_RANGE, 0, 24000000, NULL}, - {"vrc_buf_size", &lavc_param_rc_buffer_size, CONF_TYPE_INT, CONF_RANGE, 4, 24000000, NULL}, + {"vrc_maxrate", &lavc_param_rc_max_rate, CONF_TYPE_INT, CONF_RANGE, 0, MAX_BITRATE, NULL}, + {"vrc_minrate", &lavc_param_rc_min_rate, CONF_TYPE_INT, CONF_RANGE, 0, MAX_BITRATE, NULL}, + {"vrc_buf_size", &lavc_param_rc_buffer_size, CONF_TYPE_INT, CONF_RANGE, 4, MAX_BITRATE, NULL}, {"vrc_buf_aggressivity", &lavc_param_rc_buffer_aggressivity, CONF_TYPE_FLOAT, CONF_RANGE, 0.0, 99.0, NULL}, {"vrc_init_cplx", &lavc_param_rc_initial_cplx, CONF_TYPE_FLOAT, CONF_RANGE, 0.0, 9999999.0, NULL}, {"vrc_init_occupancy", &lavc_param_rc_initial_buffer_occupancy, CONF_TYPE_FLOAT, CONF_RANGE, 0.0, 1.0, NULL},
On Tue, Jan 04, 2011 at 09:35:02PM +0100, Tobias Diedrich wrote:
This about quadruples the maximum from 24Mbit to to 100Mbit. Maybe use INT_MAX or something to not limit the user in any way?
Original patch by Krishna Gadepalli and Debargha Mukherjee.
Signed-off-by: Tobias Diedrich <ranma@google.com>
That's fine, I had similar things in my tree but could never decide on a value. What speaks against INT_MAX is that there are likely to be some integer-overflows and encoding would break horribly if you actually used that.
Use limits.h to get the maximum length instead of hardcoding it. Original patch by Sang-Uok Kum. Signed-off-by: Tobias Diedrich <ranma@google.com> Index: mplayer-patchset1/libmpdemux/mf.c =================================================================== --- mplayer-patchset1.orig/libmpdemux/mf.c 2010-12-21 20:05:53.375548000 +0100 +++ mplayer-patchset1/libmpdemux/mf.c 2010-12-21 20:43:53.699417000 +0100 @@ -23,6 +23,7 @@ #include <unistd.h> #include <fcntl.h> #include <errno.h> +#include <limits.h> #include <sys/types.h> #include <sys/stat.h> @@ -63,8 +64,8 @@ FILE *lst_f=fopen(filename + 1,"r"); if ( lst_f ) { - fname=malloc( 255 ); - while ( fgets( fname,255,lst_f ) ) + fname=malloc(PATH_MAX); + while ( fgets( fname,PATH_MAX,lst_f ) ) { /* remove spaces from end of fname */ char *t=fname + strlen( fname ) - 1;
Hi, On Tue, Jan 04, 2011 at 09:35:03PM +0100, Tobias Diedrich wrote:
Use limits.h to get the maximum length instead of hardcoding it.
- fname=malloc( 255 ); - while ( fgets( fname,255,lst_f ) ) + fname=malloc(PATH_MAX); + while ( fgets( fname,PATH_MAX,lst_f ) )
FWIW there is no guarantee that PATH_MAX even exists. See e.g. http://insanecoding.blogspot.com/2007/11/pathmax-simply-isnt.html or http://www.gnu.org/software/hurd/community/gsoc/project_ideas/maxpath.html or http://bytes.com/topic/c/answers/215365-path_max I'd thus suggest a configure check for PATH_MAX and providing some arbitrary fallback value, e.g. 255 as used currently(?). Erik -- In the beginning, there were not enough colors. -- Guy Keren
Hi, On Wed, Jan 05, 2011 at 11:43:51AM +0100, Erik Auerswald wrote:
On Tue, Jan 04, 2011 at 09:35:03PM +0100, Tobias Diedrich wrote:
Use limits.h to get the maximum length instead of hardcoding it.
- fname=malloc( 255 ); - while ( fgets( fname,255,lst_f ) ) + fname=malloc(PATH_MAX); + while ( fgets( fname,PATH_MAX,lst_f ) )
FWIW there is no guarantee that PATH_MAX even exists. See e.g. http://insanecoding.blogspot.com/2007/11/pathmax-simply-isnt.html or http://www.gnu.org/software/hurd/community/gsoc/project_ideas/maxpath.html or http://bytes.com/topic/c/answers/215365-path_max
I'd thus suggest a configure check for PATH_MAX and providing some arbitrary fallback value, e.g. 255 as used currently(?).
The header to find PATH_MAX can differ: Mac/FreeBSD /usr/include/sys/syslimits.h Linux (Debian/Fedora Core) /usr/include/linux/limits.h Solaris /usr/include/limits.h (from http://andre.stechert.org/urwhatu/2005/10/path_max.html) Erik -- If you use a kernel.org-based kernel, and you aren't updating to the latest -stable updates, well, why are you using a kernel.org kernel in the first place? -- Greg Kroah-Hartman
On Wed, Jan 05, 2011 at 11:43:51AM +0100, Erik Auerswald wrote:
On Tue, Jan 04, 2011 at 09:35:03PM +0100, Tobias Diedrich wrote:
Use limits.h to get the maximum length instead of hardcoding it.
- fname=malloc( 255 ); - while ( fgets( fname,255,lst_f ) ) + fname=malloc(PATH_MAX); + while ( fgets( fname,PATH_MAX,lst_f ) )
FWIW there is no guarantee that PATH_MAX even exists. See e.g. http://insanecoding.blogspot.com/2007/11/pathmax-simply-isnt.html or http://www.gnu.org/software/hurd/community/gsoc/project_ideas/maxpath.html or http://bytes.com/topic/c/answers/215365-path_max
False, POSIX requires it: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/limits.h.html#tag_1... So relying on the existence of PATH_MAX is not a problem. Also, it is being used in other places already. Diego
Hi, On Wed, Jan 05, 2011 at 01:06:59PM +0100, Diego Biurrun wrote:
On Wed, Jan 05, 2011 at 11:43:51AM +0100, Erik Auerswald wrote:
On Tue, Jan 04, 2011 at 09:35:03PM +0100, Tobias Diedrich wrote:
Use limits.h to get the maximum length instead of hardcoding it.
- fname=malloc( 255 ); - while ( fgets( fname,255,lst_f ) ) + fname=malloc(PATH_MAX); + while ( fgets( fname,PATH_MAX,lst_f ) )
FWIW there is no guarantee that PATH_MAX even exists. See e.g. http://insanecoding.blogspot.com/2007/11/pathmax-simply-isnt.html or http://www.gnu.org/software/hurd/community/gsoc/project_ideas/maxpath.html or http://bytes.com/topic/c/answers/215365-path_max
False, POSIX requires it:
http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/limits.h.html#tag_1...
"A definition of one of the symbolic constants in the following list shall ^^^^^ be omitted from the <limits.h> header on specific implementations where the ^^^^^^^^^^ corresponding value is equal to or greater than the stated minimum, but where the value can vary depending on the file to which it is applied." _POSIX_PATH_MAX is required by POSIX. If the value for "PATH_MAX" is variable and not less than _POSIX_PATH_MAX there it shall not be defined.
So relying on the existence of PATH_MAX is not a problem. Also, it is being used in other places already.
So nobody is using MPlayer on Debian/GNU Hurd. ;-) All in all I don't see unconditionally using PATH_MAX as an improvement over 255. For most (relevant) systems both will work fine most of the time. Erik -- I don't want to see the state of the file when I'm editing. -- Ken Thompson
Erik Auerswald wrote:
On Wed, Jan 05, 2011 at 01:06:59PM +0100, Diego Biurrun wrote:
So relying on the existence of PATH_MAX is not a problem. Also, it is being used in other places already.
So nobody is using MPlayer on Debian/GNU Hurd. ;-)
All in all I don't see unconditionally using PATH_MAX as an improvement over 255. For most (relevant) systems both will work fine most of the time.
Well, we had to change it because we ran into the 255 character limit. Since PATH_MAX is already used in e.g. mplayer.c and other places I'm inclined to commit the patch as-is. -- Tobias PGP: http://8ef7ddba.uguu.de
Tobias Diedrich wrote:
Erik Auerswald wrote:
On Wed, Jan 05, 2011 at 01:06:59PM +0100, Diego Biurrun wrote:
So relying on the existence of PATH_MAX is not a problem. Also, it is being used in other places already.
So nobody is using MPlayer on Debian/GNU Hurd. ;-)
All in all I don't see unconditionally using PATH_MAX as an improvement over 255. For most (relevant) systems both will work fine most of the time.
Well, we had to change it because we ran into the 255 character limit. Since PATH_MAX is already used in e.g. mplayer.c and other places I'm inclined to commit the patch as-is.
Oh, and there is one definitive advantage it brings: If you want to go and introduce MP_PATH_MAX or something later, it's easy to grep for PATH_MAX, so you're less likely to miss this one. -- Tobias PGP: http://8ef7ddba.uguu.de
On Wed, Jan 05, 2011 at 03:40:02PM +0100, Tobias Diedrich wrote:
Tobias Diedrich wrote:
Erik Auerswald wrote:
On Wed, Jan 05, 2011 at 01:06:59PM +0100, Diego Biurrun wrote:
So relying on the existence of PATH_MAX is not a problem. Also, it is being used in other places already.
So nobody is using MPlayer on Debian/GNU Hurd. ;-)
All in all I don't see unconditionally using PATH_MAX as an improvement over 255. For most (relevant) systems both will work fine most of the time.
Well, we had to change it because we ran into the 255 character limit. Since PATH_MAX is already used in e.g. mplayer.c and other places I'm inclined to commit the patch as-is.
Oh, and there is one definitive advantage it brings: If you want to go and introduce MP_PATH_MAX or something later, it's easy to grep for PATH_MAX, so you're less likely to miss this one.
I'll apply this patch in a few hours. Yes PATH_MAX is better, even though it's not the correct "solution". -- Clément B.
On Wed, Jan 05, 2011 at 03:51:56PM +0100, Clément Bœsch wrote:
On Wed, Jan 05, 2011 at 03:40:02PM +0100, Tobias Diedrich wrote:
Tobias Diedrich wrote:
Erik Auerswald wrote:
On Wed, Jan 05, 2011 at 01:06:59PM +0100, Diego Biurrun wrote:
So relying on the existence of PATH_MAX is not a problem. Also, it is being used in other places already.
So nobody is using MPlayer on Debian/GNU Hurd. ;-)
All in all I don't see unconditionally using PATH_MAX as an improvement over 255. For most (relevant) systems both will work fine most of the time.
Well, we had to change it because we ran into the 255 character limit. Since PATH_MAX is already used in e.g. mplayer.c and other places I'm inclined to commit the patch as-is.
Oh, and there is one definitive advantage it brings: If you want to go and introduce MP_PATH_MAX or something later, it's easy to grep for PATH_MAX, so you're less likely to miss this one.
I'll apply this patch in a few hours. Yes PATH_MAX is better, even though it's not the correct "solution".
It is a step in the right direction, and for places that use malloc and no calculations there isn't anything speaking against it, so I agree with you applying it (though I should note there are a lot more places with this issues AFAIK).
On Wed, Jan 05, 2011 at 03:51:56PM +0100, Clément Bœsch wrote:
On Wed, Jan 05, 2011 at 03:40:02PM +0100, Tobias Diedrich wrote:
Tobias Diedrich wrote:
Erik Auerswald wrote:
On Wed, Jan 05, 2011 at 01:06:59PM +0100, Diego Biurrun wrote:
So relying on the existence of PATH_MAX is not a problem. Also, it is being used in other places already.
So nobody is using MPlayer on Debian/GNU Hurd. ;-)
All in all I don't see unconditionally using PATH_MAX as an improvement over 255. For most (relevant) systems both will work fine most of the time.
Well, we had to change it because we ran into the 255 character limit. Since PATH_MAX is already used in e.g. mplayer.c and other places I'm inclined to commit the patch as-is.
Oh, and there is one definitive advantage it brings: If you want to go and introduce MP_PATH_MAX or something later, it's easy to grep for PATH_MAX, so you're less likely to miss this one.
I'll apply this patch in a few hours. Yes PATH_MAX is better, even though it's not the correct "solution".
Tobias is an MPlayer oldtimer, he can commit himself, just so you know. Diego
On Wed, Jan 05, 2011 at 05:02:58PM +0100, Diego Biurrun wrote:
On Wed, Jan 05, 2011 at 03:51:56PM +0100, Clément Bœsch wrote:
On Wed, Jan 05, 2011 at 03:40:02PM +0100, Tobias Diedrich wrote:
Tobias Diedrich wrote:
Erik Auerswald wrote:
On Wed, Jan 05, 2011 at 01:06:59PM +0100, Diego Biurrun wrote:
So relying on the existence of PATH_MAX is not a problem. Also, it is being used in other places already.
So nobody is using MPlayer on Debian/GNU Hurd. ;-)
All in all I don't see unconditionally using PATH_MAX as an improvement over 255. For most (relevant) systems both will work fine most of the time.
Well, we had to change it because we ran into the 255 character limit. Since PATH_MAX is already used in e.g. mplayer.c and other places I'm inclined to commit the patch as-is.
Oh, and there is one definitive advantage it brings: If you want to go and introduce MP_PATH_MAX or something later, it's easy to grep for PATH_MAX, so you're less likely to miss this one.
I'll apply this patch in a few hours. Yes PATH_MAX is better, even though it's not the correct "solution".
Tobias is an MPlayer oldtimer, he can commit himself, just so you know.
Oh ok, sorry. Then I won't commit :) -- Clément B.
None of the calling sites are checking the return value to see if ALL bytes got written. This was causing (very occasionally) problems with mencoder when using output pipes AND running under a sandbox or when being straced (ptrace is the culprit) Theoretically this problem can happen without pipes or ptrace. Original patch by Sang-Uok Kum. Signed-off-by: Tobias Diedrich <ranma@google.com> Index: mplayer-patchset1/stream/stream.c =================================================================== --- mplayer-patchset1.orig/stream/stream.c 2010-12-21 20:45:31.157756000 +0100 +++ mplayer-patchset1/stream/stream.c 2010-12-21 20:48:16.229732000 +0100 @@ -324,13 +324,16 @@ } int stream_write_buffer(stream_t *s, unsigned char *buf, int len) { - int rd; - if(!s->write_buffer) + int rd = 0; + if(!s->write_buffer || len < 0) return -1; - rd = s->write_buffer(s, buf, len); - if(rd < 0) - return -1; - s->pos += rd; + while (rd < len) { + int ret = s->write_buffer(s, buf+rd, len-rd); + if (ret < 0) + return -1; + rd += ret; + s->pos += ret; + } return rd; }
On Tue, Jan 04, 2011 at 09:35:04PM +0100, Tobias Diedrich wrote:
None of the calling sites are checking the return value to see if ALL bytes got written.
This was causing (very occasionally) problems with mencoder when using output pipes AND running under a sandbox or when being straced (ptrace is the culprit) Theoretically this problem can happen without pipes or ptrace.
Original patch by Sang-Uok Kum.
Signed-off-by: Tobias Diedrich <ranma@google.com>
Index: mplayer-patchset1/stream/stream.c =================================================================== --- mplayer-patchset1.orig/stream/stream.c 2010-12-21 20:45:31.157756000 +0100 +++ mplayer-patchset1/stream/stream.c 2010-12-21 20:48:16.229732000 +0100 @@ -324,13 +324,16 @@ }
int stream_write_buffer(stream_t *s, unsigned char *buf, int len) { - int rd; - if(!s->write_buffer) + int rd = 0; + if(!s->write_buffer || len < 0) return -1; - rd = s->write_buffer(s, buf, len); - if(rd < 0) - return -1; - s->pos += rd; + while (rd < len) { + int ret = s->write_buffer(s, buf+rd, len-rd); + if (ret < 0) + return -1; + rd += ret; + s->pos += ret; + }
I think this is a risky place to do it, I'd be more in favour of only adding an assert here and fix all write_buffer implementations to make sure this issue never happens. There is also the issue that if write_buffer ever ends up returning 0 this would become an endless loop.
Reimar Döffinger wrote:
On Tue, Jan 04, 2011 at 09:35:04PM +0100, Tobias Diedrich wrote:
Index: mplayer-patchset1/stream/stream.c =================================================================== --- mplayer-patchset1.orig/stream/stream.c 2010-12-21 20:45:31.157756000 +0100 +++ mplayer-patchset1/stream/stream.c 2010-12-21 20:48:16.229732000 +0100 @@ -324,13 +324,16 @@ }
int stream_write_buffer(stream_t *s, unsigned char *buf, int len) { - int rd; - if(!s->write_buffer) + int rd = 0; + if(!s->write_buffer || len < 0) return -1; - rd = s->write_buffer(s, buf, len); - if(rd < 0) - return -1; - s->pos += rd; + while (rd < len) { + int ret = s->write_buffer(s, buf+rd, len-rd); + if (ret < 0) + return -1; + rd += ret; + s->pos += ret; + }
I think this is a risky place to do it, I'd be more in favour of only adding an assert here and fix all write_buffer implementations to make sure this issue never happens. There is also the issue that if write_buffer ever ends up returning 0 this would become an endless loop.
Something more like this? Index: mplayer-patchset1/stream/stream.c =================================================================== --- mplayer-patchset1.orig/stream/stream.c 2011-01-06 16:16:21.982600000 +0100 +++ mplayer-patchset1/stream/stream.c 2011-01-06 16:47:28.561260000 +0100 @@ -28,6 +28,7 @@ #endif #include <fcntl.h> #include <strings.h> +#include <assert.h> #include "config.h" @@ -331,6 +332,7 @@ if(rd < 0) return -1; s->pos += rd; + assert(rd == len && "stream_write_buffer(): unexpected short write"); return rd; } Index: mplayer-patchset1/stream/stream_file.c =================================================================== --- mplayer-patchset1.orig/stream/stream_file.c 2011-01-06 16:18:13.945756000 +0100 +++ mplayer-patchset1/stream/stream_file.c 2011-01-06 16:45:35.911193000 +0100 @@ -52,13 +52,32 @@ }; static int fill_buffer(stream_t *s, char* buffer, int max_len){ - int r = read(s->fd,buffer,max_len); - return (r <= 0) ? -1 : r; + int nr = 0; + int r; + while (nr < max_len) { + r = read(s->fd,buffer,max_len); + if (r < 0) + return -1; + max_len -= r; + buffer += r; + nr += r; + if (r == 0) + break; + } + return nr; } static int write_buffer(stream_t *s, char* buffer, int len) { - int r = write(s->fd,buffer,len); - return (r <= 0) ? -1 : r; + int r; + int wr = 0; + while (wr < len) { + r = write(s->fd,buffer,len); + if (r <= 0) + return -1; + wr += r; + buffer += r; + } + return len; } static int seek(stream_t *s,off_t newpos) { -- Tobias PGP: http://8ef7ddba.uguu.de
On Thu, Jan 06, 2011 at 05:06:33PM +0100, Tobias Diedrich wrote:
Reimar Döffinger wrote:
On Tue, Jan 04, 2011 at 09:35:04PM +0100, Tobias Diedrich wrote:
Index: mplayer-patchset1/stream/stream.c =================================================================== --- mplayer-patchset1.orig/stream/stream.c 2010-12-21 20:45:31.157756000 +0100 +++ mplayer-patchset1/stream/stream.c 2010-12-21 20:48:16.229732000 +0100 @@ -324,13 +324,16 @@ }
int stream_write_buffer(stream_t *s, unsigned char *buf, int len) { - int rd; - if(!s->write_buffer) + int rd = 0; + if(!s->write_buffer || len < 0) return -1; - rd = s->write_buffer(s, buf, len); - if(rd < 0) - return -1; - s->pos += rd; + while (rd < len) { + int ret = s->write_buffer(s, buf+rd, len-rd); + if (ret < 0) + return -1; + rd += ret; + s->pos += ret; + }
I think this is a risky place to do it, I'd be more in favour of only adding an assert here and fix all write_buffer implementations to make sure this issue never happens. There is also the issue that if write_buffer ever ends up returning 0 this would become an endless loop.
Something more like this?
Index: mplayer-patchset1/stream/stream.c =================================================================== --- mplayer-patchset1.orig/stream/stream.c 2011-01-06 16:16:21.982600000 +0100 +++ mplayer-patchset1/stream/stream.c 2011-01-06 16:47:28.561260000 +0100 @@ -28,6 +28,7 @@ #endif #include <fcntl.h> #include <strings.h> +#include <assert.h>
#include "config.h"
@@ -331,6 +332,7 @@ if(rd < 0) return -1; s->pos += rd; + assert(rd == len && "stream_write_buffer(): unexpected short write"); return rd; }
Index: mplayer-patchset1/stream/stream_file.c =================================================================== --- mplayer-patchset1.orig/stream/stream_file.c 2011-01-06 16:18:13.945756000 +0100 +++ mplayer-patchset1/stream/stream_file.c 2011-01-06 16:45:35.911193000 +0100 @@ -52,13 +52,32 @@ };
static int fill_buffer(stream_t *s, char* buffer, int max_len){ - int r = read(s->fd,buffer,max_len); - return (r <= 0) ? -1 : r; + int nr = 0; + int r; + while (nr < max_len) { + r = read(s->fd,buffer,max_len); + if (r < 0) + return -1; + max_len -= r; + buffer += r; + nr += r; + if (r == 0) + break; + } + return nr; }
Have you seen cases where this is necessary for read? The reason I dislike it is because I think it might interfere with the code that tries to do a clean exit on CTRL+C. Also it looks to me like your code allows a 0 return value (which previously was not possible), IIRC that can cause hangs.
Reimar Döffinger wrote:
On Thu, Jan 06, 2011 at 05:06:33PM +0100, Tobias Diedrich wrote:
Index: mplayer-patchset1/stream/stream_file.c =================================================================== --- mplayer-patchset1.orig/stream/stream_file.c 2011-01-06 16:18:13.945756000 +0100 +++ mplayer-patchset1/stream/stream_file.c 2011-01-06 16:45:35.911193000 +0100 @@ -52,13 +52,32 @@ };
static int fill_buffer(stream_t *s, char* buffer, int max_len){ - int r = read(s->fd,buffer,max_len); - return (r <= 0) ? -1 : r; + int nr = 0; + int r; + while (nr < max_len) { + r = read(s->fd,buffer,max_len); + if (r < 0) + return -1; + max_len -= r; + buffer += r; + nr += r; + if (r == 0) + break; + } + return nr; }
Have you seen cases where this is necessary for read? The reason I dislike it is because I think it might interfere with the code that tries to do a clean exit on CTRL+C. Also it looks to me like your code allows a 0 return value (which previously was not possible), IIRC that can cause hangs.
Well, given that I was adding retry code for write I thought I should look at the read codepath too. Posix spec says that read() too may be interrupted by signals and then return less than requested (probably won't happen on Linux since reads should be blocking) and of course it can happen on pipes/fifos. I'll add a check for 0 before the return. -- Tobias PGP: http://8ef7ddba.uguu.de
On 6 jan 2011, at 19:30, Tobias Diedrich <ranma@tdiedrich.de> wrote:
Reimar Döffinger wrote:
On Thu, Jan 06, 2011 at 05:06:33PM +0100, Tobias Diedrich wrote:
Index: mplayer-patchset1/stream/stream_file.c =================================================================== --- mplayer-patchset1.orig/stream/stream_file.c 2011-01-06 16:18:13.945756000 +0100 +++ mplayer-patchset1/stream/stream_file.c 2011-01-06 16:45:35.911193000 +0100 @@ -52,13 +52,32 @@ };
static int fill_buffer(stream_t *s, char* buffer, int max_len){ - int r = read(s->fd,buffer,max_len); - return (r <= 0) ? -1 : r; + int nr = 0; + int r; + while (nr < max_len) { + r = read(s->fd,buffer,max_len); + if (r < 0) + return -1; + max_len -= r; + buffer += r; + nr += r; + if (r == 0) + break; + } + return nr; }
Have you seen cases where this is necessary for read? The reason I dislike it is because I think it might interfere with the code that tries to do a clean exit on CTRL+C. Also it looks to me like your code allows a 0 return value (which previously was not possible), IIRC that can cause hangs.
Well, given that I was adding retry code for write I thought I should look at the read codepath too. Posix spec says that read() too may be interrupted by signals and then return less than requested (probably won't happen on Linux since reads should be blocking)
All signals are fatal for MPlayer, however there's a risk your patch makes it more likely to get stuck in a loop so it can't shutdown cleanly.
and of course it can happen on pipes/fifos.
Why? For writes there are good reasons, but I see no reason why reads from pipes or fifos should ever return only partial data (that kind of info seems rather hard to find though).
Reimar Döffinger wrote:
On 6 jan 2011, at 19:30, Tobias Diedrich <ranma@tdiedrich.de> wrote:
Reimar Döffinger wrote:
On Thu, Jan 06, 2011 at 05:06:33PM +0100, Tobias Diedrich wrote:
Index: mplayer-patchset1/stream/stream_file.c =================================================================== --- mplayer-patchset1.orig/stream/stream_file.c 2011-01-06 16:18:13.945756000 +0100 +++ mplayer-patchset1/stream/stream_file.c 2011-01-06 16:45:35.911193000 +0100 @@ -52,13 +52,32 @@ };
static int fill_buffer(stream_t *s, char* buffer, int max_len){ - int r = read(s->fd,buffer,max_len); - return (r <= 0) ? -1 : r; + int nr = 0; + int r; + while (nr < max_len) { + r = read(s->fd,buffer,max_len); + if (r < 0) + return -1; + max_len -= r; + buffer += r; + nr += r; + if (r == 0) + break; + } + return nr; }
Have you seen cases where this is necessary for read? The reason I dislike it is because I think it might interfere with the code that tries to do a clean exit on CTRL+C. Also it looks to me like your code allows a 0 return value (which previously was not possible), IIRC that can cause hangs.
Well, given that I was adding retry code for write I thought I should look at the read codepath too. Posix spec says that read() too may be interrupted by signals and then return less than requested (probably won't happen on Linux since reads should be blocking)
All signals are fatal for MPlayer, however there's a risk your patch makes it more likely to get stuck in a loop so it can't shutdown cleanly.
and of course it can happen on pipes/fifos.
Why? For writes there are good reasons, but I see no reason why reads from pipes or fifos should ever return only partial data (that kind of info seems rather hard to find though).
Maybe a bit of a contrieved example, but: (echo foo; sleep 1; echo bar) > test.fifo If I start a read() on the fifo before executing this, the first read returns "foo" and I have to call read a second time to get the second half. So if we want stream_file to work reliably on pipes/fifos this would be needed I think. Using the following test program I get: |ranma@navi:~$ ./fifotest |nr=3 |nr=3 #include <stdio.h> #include <stdlib.h> #include <unistd.h> #include <string.h> #include <errno.h> #include <sys/fcntl.h> #include <sys/types.h> #include <sys/stat.h> int main(int argc, char **argv) { int pid; int pfd[2]; int nr; char buf[4096]; if (pipe(pfd) == -1) { perror("pipe"); return EXIT_FAILURE; } pid = fork(); if (pid == -1) { perror("fork"); return EXIT_FAILURE; } if (pid == 0) { close(pfd[0]); write(pfd[1], "foo", 3); sleep(1); write(pfd[1], "bar", 3); close(pfd[1]); } else { close(pfd[1]); nr = read(pfd[0], buf, sizeof(buf)); printf("nr=%d\n", nr); nr = read(pfd[0], buf, sizeof(buf)); printf("nr=%d\n", nr); close(pfd[0]); } return EXIT_SUCCESS; } -- Tobias PGP: http://8ef7ddba.uguu.de
Make an aqscale parameter (equivalent to -aq option in ffmpeg) available in mencoder to get bitrate control for vorbis to work. Original patch by Debargha Mukherjee. Signed-off-by: Tobias Diedrich <ranma@google.com> Index: mplayer-patchset1/libmpcodecs/ae_lavc.c =================================================================== --- mplayer-patchset1.orig/libmpcodecs/ae_lavc.c 2010-12-21 21:17:42.269376000 +0100 +++ mplayer-patchset1/libmpcodecs/ae_lavc.c 2010-12-22 11:35:54.133756000 +0100 @@ -195,6 +195,12 @@ lavc_actx->bit_rate = encoder->params.bitrate = lavc_param_abitrate * 1000; else lavc_actx->bit_rate = encoder->params.bitrate = lavc_param_abitrate; + + if(lavc_param_aqscale >= 0.0){ + lavc_actx->flags |= CODEC_FLAG_QSCALE; + lavc_actx->global_quality = (int)(FF_QP2LAMBDA * lavc_param_aqscale + 0.5); + } + if(lavc_param_audio_avopt){ if(parse_avopts(lavc_actx, lavc_param_audio_avopt) < 0){ mp_msg(MSGT_MENCODER,MSGL_ERR, "Your options /%s/ look like gibberish to me pal\n", lavc_param_audio_avopt); Index: mplayer-patchset1/libmpcodecs/ve.h =================================================================== --- mplayer-patchset1.orig/libmpcodecs/ve.h 2010-12-21 21:17:42.279364000 +0100 +++ mplayer-patchset1/libmpcodecs/ve.h 2010-12-22 11:35:54.135757000 +0100 @@ -28,6 +28,7 @@ extern char *lavc_param_acodec; extern char *lavc_param_audio_avopt; extern int lavc_param_abitrate; +extern float lavc_param_aqscale; extern int lavc_param_atag; extern int lavc_param_audio_global_header; Index: mplayer-patchset1/libmpcodecs/ve_lavc.c =================================================================== --- mplayer-patchset1.orig/libmpcodecs/ve_lavc.c 2010-12-22 11:35:53.918760000 +0100 +++ mplayer-patchset1/libmpcodecs/ve_lavc.c 2010-12-22 11:35:54.139757000 +0100 @@ -177,6 +177,7 @@ char *lavc_param_acodec = "mp2"; int lavc_param_atag = 0; int lavc_param_abitrate = 224; +float lavc_param_aqscale = -1; int lavc_param_audio_global_header= 0; static char *lavc_param_avopt = NULL; char *lavc_param_audio_avopt = NULL; @@ -188,6 +189,7 @@ const m_option_t lavcopts_conf[]={ {"acodec", &lavc_param_acodec, CONF_TYPE_STRING, 0, 0, 0, NULL}, {"abitrate", &lavc_param_abitrate, CONF_TYPE_INT, CONF_RANGE, 1, 1000000, NULL}, + {"aqscale", &lavc_param_aqscale, CONF_TYPE_FLOAT, CONF_RANGE, 0.0, 99999.0, NULL}, {"atag", &lavc_param_atag, CONF_TYPE_INT, CONF_RANGE, 0, 0xffff, NULL}, {"vcodec", &lavc_param_vcodec, CONF_TYPE_STRING, 0, 0, 0, NULL}, {"vbitrate", &lavc_param_vbitrate, CONF_TYPE_INT, CONF_RANGE, 4, MAX_BITRATE, NULL}, Index: mplayer-patchset1/DOCS/man/de/mplayer.1 =================================================================== --- mplayer-patchset1.orig/DOCS/man/de/mplayer.1 2010-12-22 12:14:27.048185000 +0100 +++ mplayer-patchset1/DOCS/man/de/mplayer.1 2010-12-22 12:16:42.474528000 +0100 @@ -8190,6 +8190,10 @@ Audiobitrate in kbps (Standard: 224) . .TP +.B aqscale=<Wert> +Audioqualität (Codec-spezifisch) +. +.TP .B atag=<Wert> Benutze die angegebene Windows-Audioformatkennung (z.B.\& atag=0x55). . Index: mplayer-patchset1/DOCS/man/en/mplayer.1 =================================================================== --- mplayer-patchset1.orig/DOCS/man/en/mplayer.1 2010-12-22 12:14:24.251421000 +0100 +++ mplayer-patchset1/DOCS/man/en/mplayer.1 2010-12-22 12:17:09.607385000 +0100 @@ -8322,6 +8322,10 @@ audio bitrate in kbps (default: 224) . .TP +.B aqscale=<value> +audio quality (codec-specific) +. +.TP .B atag=<value> Use the specified Windows audio format tag (e.g.\& atag=0x55). .
On 4 jan 2011, at 21:35, Tobias Diedrich <ranma@tdiedrich.de> wrote:
Make an aqscale parameter (equivalent to -aq option in ffmpeg) available in mencoder to get bitrate control for vorbis to work.
You can set all FFmpeg options using -lavcopts o=..., all other options are only there for backwards-compatibility (I haven't checked if it is available for ae, but if not it should be implemented).
Reimar Döffinger wrote:
On 4 jan 2011, at 21:35, Tobias Diedrich <ranma@tdiedrich.de> wrote:
Make an aqscale parameter (equivalent to -aq option in ffmpeg) available in mencoder to get bitrate control for vorbis to work.
You can set all FFmpeg options using -lavcopts o=..., all other options are only there for backwards-compatibility (I haven't checked if it is available for ae, but if not it should be implemented).
Thanks, I'll look into that. -- Tobias PGP: http://8ef7ddba.uguu.de
Original patch by Sang-Uok Kum. Signed-off-by: Tobias Diedrich <ranma@google.com> Index: mplayer-patchset1/libmpdemux/muxer.c =================================================================== --- mplayer-patchset1.orig/libmpdemux/muxer.c 2010-12-21 21:03:16.199700000 +0100 +++ mplayer-patchset1/libmpdemux/muxer.c 2010-12-21 21:05:11.009615000 +0100 @@ -140,6 +140,8 @@ s->timer = buf->dts; s->buffer = buf->buffer; s->muxer->cont_write_chunk(s, buf->len, buf->flags, buf->dts, buf->pts); + free(buf->buffer); + buf->buffer = NULL; /* 3. restore saved timer and buffer */ s->timer = tmp_buf.dts; @@ -148,6 +150,7 @@ free(s->muxer->muxbuf); s->muxer->muxbuf_num = 0; + s->muxer->muxbuf = NULL; } }
On Tue, Jan 04, 2011 at 09:35:06PM +0100, Tobias Diedrich wrote:
Original patch by Sang-Uok Kum.
Signed-off-by: Tobias Diedrich <ranma@google.com>
Index: mplayer-patchset1/libmpdemux/muxer.c =================================================================== --- mplayer-patchset1.orig/libmpdemux/muxer.c 2010-12-21 21:03:16.199700000 +0100 +++ mplayer-patchset1/libmpdemux/muxer.c 2010-12-21 21:05:11.009615000 +0100 @@ -140,6 +140,8 @@ s->timer = buf->dts; s->buffer = buf->buffer; s->muxer->cont_write_chunk(s, buf->len, buf->flags, buf->dts, buf->pts); + free(buf->buffer); + buf->buffer = NULL;
I suspect the original idea was to reuse the allocated memory if possible, but that is sure better than a memleak.
@@ -148,6 +150,7 @@
free(s->muxer->muxbuf); s->muxer->muxbuf_num = 0; + s->muxer->muxbuf = NULL;
That is not a memleak fix, though it is a fix. However I see several more issues, for example I think the code still can end up with a memleak and some unwritten final packets, there needs to be some function that flushes all remaining buffered frames at the end. And after that it would be better to _not_ free muxbuf here but instead give realloc a chance to reuse the memory area.
Reimar Döffinger wrote:
On Tue, Jan 04, 2011 at 09:35:06PM +0100, Tobias Diedrich wrote:
Original patch by Sang-Uok Kum.
Signed-off-by: Tobias Diedrich <ranma@google.com>
Index: mplayer-patchset1/libmpdemux/muxer.c =================================================================== --- mplayer-patchset1.orig/libmpdemux/muxer.c 2010-12-21 21:03:16.199700000 +0100 +++ mplayer-patchset1/libmpdemux/muxer.c 2010-12-21 21:05:11.009615000 +0100 @@ -140,6 +140,8 @@ s->timer = buf->dts; s->buffer = buf->buffer; s->muxer->cont_write_chunk(s, buf->len, buf->flags, buf->dts, buf->pts); + free(buf->buffer); + buf->buffer = NULL;
I suspect the original idea was to reuse the allocated memory if possible, but that is sure better than a memleak.
@@ -148,6 +150,7 @@
free(s->muxer->muxbuf); s->muxer->muxbuf_num = 0; + s->muxer->muxbuf = NULL;
That is not a memleak fix, though it is a fix. However I see several more issues, for example I think the code still can end up with a memleak and some unwritten final packets, there needs to be some function that flushes all remaining buffered frames at the end. And after that it would be better to _not_ free muxbuf here but instead give realloc a chance to reuse the memory area.
Actually the realloc is for s->muxer->muxbuf, which already had the free. The missing free is for the buf->buffer chunk buffers. The way I see it the muxer works like this: 1) When the muxer is first allocated in muxer_new_muxer(), muxbuf_skip_buffer is initialized to 0 by calloc(). 2) muxer_write_chunk() then allocates memory buffers for all writes until and does not actually call cont_write_chunk() until all streams have at least one chunk buffered. 3) On the next muxer_write_chunk() muxbuf_skip_buffer is then set to 1 and it calls muxer_stream_fix_parameters(), muxer_write_header() and proceeds to flush those buffers, but fails to frees the buffers. 4) Since muxbuf_skip_buffer is now 1, the buffering logic is bypassed. So to me it seems like the
+ free(buf->buffer); + buf->buffer = NULL; is a valid fix unless I'm missing something.
Cheers, -- Tobias PGP: http://8ef7ddba.uguu.de
participants (5)
-
Clément Bœsch -
Diego Biurrun -
Erik Auerswald -
Reimar Döffinger -
Tobias Diedrich