Skip to content

ext/gd: adding imagecopyrotated. #11519

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

devnexen
Copy link
Member

copying an image area from a defined center position with an angle.

@nielsdos
Copy link
Member

nielsdos commented Jun 23, 2023

FWIW, I tried this on my system and it didn't work because of a ZPP arginfo mismatch. I used the following patch to fix it:
(I could be completely wrong ofc)

diff --git a/ext/gd/gd.c b/ext/gd/gd.c
index b2084dba3d..6309088d00 100644
--- a/ext/gd/gd.c
+++ b/ext/gd/gd.c
@@ -1062,9 +1062,9 @@ PHP_FUNCTION(imagecopyrotated)
 	zend_long SX, SY, SW, SH, A;
 	gdImagePtr im_dst, im_src;
 	int srcH, srcW, srcY, srcX, angle;
-	double dstX, dstY;
+	zend_long dstX, dstY;
 
-	if (zend_parse_parameters(ZEND_NUM_ARGS(), "OOddlllll", &DIM, gd_image_ce, &SIM, gd_image_ce, &dstX, &dstY, &SX, &SY, &SW, &SH, &A) == FAILURE) {
+	if (zend_parse_parameters(ZEND_NUM_ARGS(), "OOlllllll", &DIM, gd_image_ce, &SIM, gd_image_ce, &dstX, &dstY, &SX, &SY, &SW, &SH, &A) == FAILURE) {
 		RETURN_THROWS();
 	}
 
diff --git a/ext/gd/gd.stub.php b/ext/gd/gd.stub.php
index 93821b6182..c1e180bce7 100644
--- a/ext/gd/gd.stub.php
+++ b/ext/gd/gd.stub.php
@@ -521,7 +521,7 @@ function imagecolorexactalpha(GdImage $image, int $red, int $green, int $blue, i
 function imagecopyresampled(GdImage $dst_image, GdImage $src_image, int $dst_x, int $dst_y, int $src_x, int $src_y, int $dst_width, int $dst_height, int $src_width, int $src_height): bool {}
 
 #ifdef HAVE_GD_COPY_ROTATED
-function imagecopyrotated(GdImage $dst_image, GdImage $src_image, int $dst_x, int $dst_y, int $src_x, int $src_y, int $dst_width, int $dst_height, int $src_width, int $src_height, int $angle): bool {}
+function imagecopyrotated(GdImage $dst_image, GdImage $src_image, int $dst_x, int $dst_y, int $src_x, int $src_y, int $src_width, int $src_height, int $angle): bool {}
 #endif
 
 #ifdef PHP_WIN32
diff --git a/ext/gd/gd_arginfo.h b/ext/gd/gd_arginfo.h
index 1e3a0fb914..8671ee395d 100644
--- a/ext/gd/gd_arginfo.h
+++ b/ext/gd/gd_arginfo.h
@@ -1,5 +1,5 @@
 /* This is a generated file, edit the .stub.php file instead.
- * Stub hash: 26b28dde3a4b4eeca374780d8972adb14bb6e795 */
+ * Stub hash: 8235f4095cd07d7b6936be153fa7c537bc860f8b */
 
 ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_gd_info, 0, 0, IS_ARRAY, 0)
 ZEND_END_ARG_INFO()
@@ -107,15 +107,13 @@ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_imagecopyresampled, 0, 10, _IS_B
 ZEND_END_ARG_INFO()
 
 #if defined(HAVE_GD_COPY_ROTATED)
-ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_imagecopyrotated, 0, 11, _IS_BOOL, 0)
+ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_imagecopyrotated, 0, 9, _IS_BOOL, 0)
 	ZEND_ARG_OBJ_INFO(0, dst_image, GdImage, 0)
 	ZEND_ARG_OBJ_INFO(0, src_image, GdImage, 0)
 	ZEND_ARG_TYPE_INFO(0, dst_x, IS_LONG, 0)
 	ZEND_ARG_TYPE_INFO(0, dst_y, IS_LONG, 0)
 	ZEND_ARG_TYPE_INFO(0, src_x, IS_LONG, 0)
 	ZEND_ARG_TYPE_INFO(0, src_y, IS_LONG, 0)
-	ZEND_ARG_TYPE_INFO(0, dst_width, IS_LONG, 0)
-	ZEND_ARG_TYPE_INFO(0, dst_height, IS_LONG, 0)
 	ZEND_ARG_TYPE_INFO(0, src_width, IS_LONG, 0)
 	ZEND_ARG_TYPE_INFO(0, src_height, IS_LONG, 0)
 	ZEND_ARG_TYPE_INFO(0, angle, IS_LONG, 0)
@devnexen
Copy link
Member Author

in fact it s the other way around the stub is wrong.

@@ -520,6 +520,10 @@ function imagecolorexactalpha(GdImage $image, int $red, int $green, int $blue, i

function imagecopyresampled(GdImage $dst_image, GdImage $src_image, int $dst_x, int $dst_y, int $src_x, int $src_y, int $dst_width, int $dst_height, int $src_width, int $src_height): bool {}

#ifdef HAVE_GD_COPY_ROTATED
function imagecopyrotated(GdImage $dst_image, GdImage $src_image, float $dst_x, float $dst_y, int $src_x, int $src_y, int $src_width, int $src_height, int $angle): bool {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

angle should be float

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I follow gd api here.

@devnexen devnexen requested a review from Girgias July 3, 2023 17:11
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what to think of the API, but then other GD functions do pass the destination object too...

ext/gd/gd.c Outdated
Comment on lines 1059 to 1107
PHP_FUNCTION(imagecopyrotated)
{
zval *SIM, *DIM;
zend_long SX, SY, SW, SH, A;
gdImagePtr im_dst, im_src;
int srcH, srcW, srcY, srcX, angle;
double dstX, dstY;

if (zend_parse_parameters(ZEND_NUM_ARGS(), "OOddlllll", &DIM, gd_image_ce, &SIM, gd_image_ce, &dstX, &dstY, &SX, &SY, &SW, &SH, &A) == FAILURE) {
RETURN_THROWS();
}

im_dst = php_gd_libgdimageptr_from_zval_p(DIM);
im_src = php_gd_libgdimageptr_from_zval_p(SIM);

srcX = SX;
srcY = SY;
srcH = SH;
srcW = SW;
angle = A;

gdImageCopyRotated(im_dst, im_src, dstX, dstY, srcX, srcY, srcW, srcH, angle);
RETURN_TRUE;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't really make any sense to take the destination Image as an argument, as I frankly imagine one will not have pre allocated such an object.

How do other functions from the GD extension work?

Suggested change
PHP_FUNCTION(imagecopyrotated)
{
zval *SIM, *DIM;
zend_long SX, SY, SW, SH, A;
gdImagePtr im_dst, im_src;
int srcH, srcW, srcY, srcX, angle;
double dstX, dstY;
if (zend_parse_parameters(ZEND_NUM_ARGS(), "OOddlllll", &DIM, gd_image_ce, &SIM, gd_image_ce, &dstX, &dstY, &SX, &SY, &SW, &SH, &A) == FAILURE) {
RETURN_THROWS();
}
im_dst = php_gd_libgdimageptr_from_zval_p(DIM);
im_src = php_gd_libgdimageptr_from_zval_p(SIM);
srcX = SX;
srcY = SY;
srcH = SH;
srcW = SW;
angle = A;
gdImageCopyRotated(im_dst, im_src, dstX, dstY, srcX, srcY, srcW, srcH, angle);
RETURN_TRUE;
}
PHP_FUNCTION(imagecopyrotated)
{
zval *SIM;
zend_long SX, SY, SW, SH, A;
gdImagePtr im_dst, im_src;
int srcH, srcW, srcY, srcX, angle;
double dstX, dstY;
if (zend_parse_parameters(ZEND_NUM_ARGS(), "Oddlllll", &SIM, gd_image_ce, &dstX, &dstY, &SX, &SY, &SW, &SH, &A) == FAILURE) {
RETURN_THROWS();
}
im_dst = create_new_object_here();
im_src = php_gd_libgdimageptr_from_zval_p(SIM);
srcX = SX;
srcY = SY;
srcH = SH;
srcW = SW;
angle = A;
gdImageCopyRotated(im_dst, im_src, dstX, dstY, srcX, srcY, srcW, srcH, angle);
RETURN_NEW_OBJECT();
}

EDIT: Okay I see that other GD functions already do this, but am I the only one to find this stupid or? In any case if the function can only fail when throwing an exception make the function void.

@devnexen
Copy link
Member Author

cc @Girgias, possibly before the freeze 🙏🙂

Comment on lines +1079 to +1106

gdImageCopyRotated(im_dst, im_src, dstX, dstY, srcX, srcY, srcW, srcH, angle);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only thing would be to check that the zend_long being passed fits into the value storable by a C int and if not throw a ValueError like we do in other functions

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does it look to you now ?

devnexen added 2 commits July 13, 2023 14:28
copying an image area from a defined center position with an angle.
@cmb69
Copy link
Member

cmb69 commented Jul 14, 2024

If we add this function, we should also add gdImageCopyRotated() to our bundled libgd, in my opinion.

However, I'm not sure we should add more of the old-style gdImageCopy*() functions; in this case we already have imagerotate() (which uses the newer gdImageRotateInterpolated() which is more flexible).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment