[MPlayer-dev-eng] [PATCH] aspect and round params for dsize
Oded Shimon
ods15 at ods15.dyndns.org
Tue Jul 19 19:08:49 CEST 2005
On Sun, Jul 10, 2005 at 03:01:20PM +0200, Attila Kinali wrote:
> On Sat, 9 Jul 2005 14:55:01 +0300
> Oded Shimon <ods15 at ods15.dyndns.org> wrote:
>
> > [dsize.patch text/plain (5096 bytes)]
> > Index: libmpcodecs/vf_dsize.c
> > ===================================================================
> > RCS file: /cvsroot/mplayer/main/libmpcodecs/vf_dsize.c,v
> > retrieving revision 1.1
> > diff -u -r1.1 vf_dsize.c
> > --- libmpcodecs/vf_dsize.c 27 Apr 2003 18:55:04 -0000 1.1
> > +++ libmpcodecs/vf_dsize.c 4 Jul 2005 09:01:25 -0000
> > @@ -12,6 +12,7 @@
> >
> > struct vf_priv_s {
> > int w, h;
> > + int a, r;
>
> while w and h are imediatly clear in the vo context, what
> are a and r ? Please add at least a comment here or even better
> use longer variable names and comment them
Good point. Fixed.
> > float aspect;
> > };
> >
> > @@ -19,7 +20,27 @@
> > int width, int height, int d_width, int d_height,
> > unsigned int flags, unsigned int outfmt)
> > {
> > - if (vf->priv->w && vf->priv->h) {
> > + if (vf->priv->aspect < 0.001) {
>
> Where does the value 0.001 come from ?
Floats are fragile. checking for 0.0 would work, but isn't 100% reliable.
> What do we exactly check here ?
> Why isn't there any comment ?
Added.
> > + if (vf->priv->w == 0) vf->priv->w = d_width;
> > + if (vf->priv->h == 0) vf->priv->h = d_height;
> > + if (vf->priv->w == -1) vf->priv->w = width;
> > + if (vf->priv->h == -1) vf->priv->h = height;
> > + if (vf->priv->w == -2) vf->priv->w = vf->priv->h * (double)d_width/d_height;
> > + if (vf->priv->w == -3) vf->priv->w = vf->priv->h * (double)width / height;
> > + if (vf->priv->h == -2) vf->priv->h = vf->priv->w * (double)d_height / d_width;
> > + if (vf->priv->h == -3) vf->priv->h = vf->priv->w * (double)height / width;
>
> What happens here if both h and w are negative ?
Look at boundry checks in init()...
> > + if (vf->priv->a > -1) { // 0 -> downscale, 1-> upscale. +2 -> original aspect.
> > + double aspect = (vf->priv->a & 2) ? ((double)d_height / d_width) : ((double)height / width);
>
> write the condition as vf->priv->a !=0, the a&2 hardly readable
I check whether a is 1 or 3, and not 2 or 4. I could make it more explicit,
"a == 1 || a == 3"...
> > + if ((vf->priv->h > vf->priv->w * aspect) ^ (vf->priv->a & 1)) {
>
> Use constructs like this in the ioccc but not in MPlayer.
Discussed on IRC and we decided
if (a ^ b) {
1;
} else {
2;
}
is more readable than
if (a) {
if (!b) {
1;
} else {
2;
}
} else {
if (b) {
1;
} else {
2;
}
}
> > + vf->priv->h = vf->priv->w * aspect;
> > + } else {
> > + vf->priv->w = vf->priv->h / aspect;
> > + }
> > + }
> > + if (vf->priv->r > 1) {
> > + vf->priv->w += (vf->priv->r - 1 - (vf->priv->w - 1) % vf->priv->r);
> > + vf->priv->h += (vf->priv->r - 1 - (vf->priv->h - 1) % vf->priv->r);
>
> That code is fragile!
> Module operation of negative numbers does not work
> on all architectures as expected.
Again, check boundry checks in init().
Unless you're reffering to w and h being smaller than 2. In which case, I
admit this code will probably crash, but so will the vo most likely. :)
> And why do you always round down ?
Round UP! I'll add a comment saying it's // round up.
People seem to keep confusing it with round down, even though it's += ...
> Why isn't this mentioned in the manpage?
huh?
+.IPs <r>
+Rounds up to make both width and height divisible by <r> (default: 1).
> > + }
> > d_width = vf->priv->w;
> > d_height = vf->priv->h;
> > } else {
> [...]
>
> Over all: a bit too obfuscated for my taste. Learn that complicated
> code doesn't let you look smarter. But it makes maintenance
> of the project much harder.
> You should also add many more comments on what's going on,
> especialy if you are doing something not so obvious.
> You should also learn to use C-code idioms a lot more, it
> makes reading easier.
>
> And while you are at it, could you add some doxygen docs for
> the whole file? Thanks.
Bleh, no. :)
vf_'s mostly really don't deserve doxygen. vf.c on the other hand, does. :)
- ods15
-------------- next part --------------
Index: libmpcodecs/vf_dsize.c
===================================================================
RCS file: /cvsroot/mplayer/main/libmpcodecs/vf_dsize.c,v
retrieving revision 1.1
diff -u -r1.1 vf_dsize.c
--- libmpcodecs/vf_dsize.c 27 Apr 2003 18:55:04 -0000 1.1
+++ libmpcodecs/vf_dsize.c 19 Jul 2005 17:03:16 -0000
@@ -12,6 +12,8 @@
struct vf_priv_s {
int w, h;
+ int method; // aspect method, 0 -> downscale, 1-> upscale. +2 -> original aspect.
+ int round;
float aspect;
};
@@ -19,7 +20,27 @@
int width, int height, int d_width, int d_height,
unsigned int flags, unsigned int outfmt)
{
- if (vf->priv->w && vf->priv->h) {
+ if (vf->priv->aspect < 0.001) { // did the user input aspect or w,h params
+ if (vf->priv->w == 0) vf->priv->w = d_width;
+ if (vf->priv->h == 0) vf->priv->h = d_height;
+ if (vf->priv->w == -1) vf->priv->w = width;
+ if (vf->priv->h == -1) vf->priv->h = height;
+ if (vf->priv->w == -2) vf->priv->w = vf->priv->h * (double)d_width / d_height;
+ if (vf->priv->w == -3) vf->priv->w = vf->priv->h * (double)width / height;
+ if (vf->priv->h == -2) vf->priv->h = vf->priv->w * (double)d_height / d_width;
+ if (vf->priv->h == -3) vf->priv->h = vf->priv->w * (double)height / width;
+ if (vf->priv->method > -1) {
+ double aspect = (vf->priv->method & 2) ? ((double)d_height / d_width) : ((double)height / width);
+ if ((vf->priv->h > vf->priv->w * aspect) ^ (vf->priv->method & 1)) {
+ vf->priv->h = vf->priv->w * aspect;
+ } else {
+ vf->priv->w = vf->priv->h / aspect;
+ }
+ }
+ if (vf->priv->round > 1) { // round up
+ vf->priv->w += (vf->priv->round - 1 - (vf->priv->w - 1) % vf->priv->round);
+ vf->priv->h += (vf->priv->round - 1 - (vf->priv->h - 1) % vf->priv->round);
+ }
d_width = vf->priv->w;
d_height = vf->priv->h;
} else {
@@ -34,13 +55,23 @@
return vf_next_config(vf, width, height, d_width, d_height, flags, outfmt);
}
+static void uninit(vf_instance_t *vf) {
+ free(vf->priv);
+ vf->priv = NULL;
+}
+
static int open(vf_instance_t *vf, char* args)
{
vf->config = config;
vf->draw_slice = vf_next_draw_slice;
+ vf->uninit = uninit;
//vf->default_caps = 0;
vf->priv = calloc(sizeof(struct vf_priv_s), 1);
- vf->priv->aspect = 4.0/3.0;
+ vf->priv->aspect = 0.;
+ vf->priv->w = -1;
+ vf->priv->h = -1;
+ vf->priv->method = -1;
+ vf->priv->round = 1;
if (args) {
if (strchr(args, '/')) {
int w, h;
@@ -49,9 +80,17 @@
} else if (strchr(args, '.')) {
sscanf(args, "%f", &vf->priv->aspect);
} else {
- sscanf(args, "%d:%d", &vf->priv->w, &vf->priv->h);
+ sscanf(args, "%d:%d:%d:%d", &vf->priv->w, &vf->priv->h, &vf->priv->method, &vf->priv->round);
}
}
+ if ((vf->priv->aspect < 0.) || (vf->priv->w < -3) || (vf->priv->h < -3) ||
+ ((vf->priv->w < -1) && (vf->priv->h < -1)) ||
+ (vf->priv->method < -1) || (vf->priv->method > 3) ||
+ (vf->priv->round < 0)) {
+ mp_msg(MSGT_VFILTER, MSGL_ERR, "[dsize] Illegal value(s): aspect: %f w: %d h: %d aspect_method: %d round: %d\n", vf->priv->aspect, vf->priv->w, vf->priv->h, vf->priv->method, vf->priv->round);
+ free(vf->priv); vf->priv = NULL;
+ return -1;
+ }
return 1;
}
Index: DOCS/man/en/mplayer.1
===================================================================
RCS file: /cvsroot/mplayer/main/DOCS/man/en/mplayer.1,v
retrieving revision 1.1028
diff -u -r1.1028 mplayer.1
--- DOCS/man/en/mplayer.1 7 Jul 2005 20:26:56 -0000 1.1028
+++ DOCS/man/en/mplayer.1 19 Jul 2005 17:03:24 -0000
@@ -4319,7 +4319,7 @@
.RE
.
.TP
-.B dsize[=aspect|w:h]
+.B dsize[=aspect|w:h:aspect-method:r]
Changes the intended display size/\:aspect at an arbitrary point in the
filter chain.
Aspect can be given as a fraction (4/\:3) or floating point number
@@ -4331,6 +4331,58 @@
do any scaling itself; it just affects
what later scalers (software or hardware) will do when auto-scaling to
correct aspect.
+.RSs
+.IPs <w>,<h>
+New display width and height.
+Can also be these special values:
+.RSss
+ 0: original display width and height
+.br
+-1: original video width and height (default)
+.br
+-2: Calculate w/\:h using the other dimension and the original display
+aspect ratio.
+.br
+-3: Calculate w/\:h using the other dimension and the original video
+aspect ratio.
+.REss
+.sp 1
+.I EXAMPLE:
+.PD 0
+.RSs
+.IP dsize=800:-2
+Specifies a display resolution of 800x600 for a 4/3 aspect video, or
+800x450 for a 16/9 aspect video.
+.RE
+.IPs <aspect\-method>
+Modifies width and height according to original aspect ratios.
+.RSss
+-1: Ignore original aspect ratio (default).
+.br
+ 0: Keep display aspect ratio by using <w> and <h> as maximum
+resolution.
+.br
+ 1: Keep display aspect ratio by using <w> and <h> as minimum
+resolution.
+.br
+ 2: Keep video aspect ratio by using <w> and <h> as maximum
+resolution.
+.br
+ 3: Keep video aspect ratio by using <w> and <h> as minimum
+resolution.
+.REss
+.sp 1
+.I EXAMPLE:
+.PD 0
+.RSs
+.IP dsize=800:600:0
+Specifies a display resolution of at most 800x600, or smaller, in order
+to keep aspect.
+.RE
+.PD 1
+.IPs <r>
+Rounds up to make both width and height divisible by <r> (default: 1).
+.RE
.
.TP
.B yuy2\ \ \
More information about the MPlayer-dev-eng
mailing list