"Export" php_gd_libgdimageptr_from_zval_p() by cmb69 · Pull Request #5830 · php/php-src

cmb69 · 2020-07-09T19:58:46Z

Some extension may need to retrieve the gdImagePtr from an GdImage
object; thus, we move the respective function and the supporting code
to php_gd.h.

Todo: check why FFMPEG_PHP_FETCH_IMAGE_RESOURCE is still used in PHP 8

nikic · 2020-07-10T08:19:54Z

In file included from main/internal_functions_cli.c:45:
/home/vsts/work/1/s/ext/gd/php_gd.h:80:10: fatal error: gd.h: No such file or directory

I think having an include at this position means we need to pollute the global include path with the gd incdir, rather than just for the gd extension.

I don't think we actually need to export php_gd_image_object and php_gd_exgdimage_from_zobj_p though. Just php_gd_libgdimageptr_from_zval_p as a non-inline function should be enough. If you change the return value to struct gdImageStruct * we don't need the gd.h include either.

I'm afraid all this is necessary, so we can get the offsetof of the std member in a portable way. :(

Why is the offsetof needed?

To be clear, what I means is: If the implementation is part of the .c file and only exported via PHP_GD_API, why do we need to expose the offset?

Ah, understood now. Thanks! Will update the PR this evening.

Some extension may need to retrieve the `gdImagePtr` from an `GdImage` object; thus, we export the respective function. To not being forced to include gd.h in php_gd.h, we use the opaque `struct gdImageStruct *` as return type.

Jan-E · 2020-07-11T01:39:05Z

I applied your adjusted patch to PHP 8.0.0 alpha 2 and removed the imported lines https://github.com/Jan-E/php7-ffmpeg/blob/c7d3316e7c8792348a476434ff557bd0da1abc6c/ffmpeg_frame.c#L65-L93

Then I did a complete recompile of PHP with all the extensions. Result for php_ffmpeg:

ffmpeg_frame.obj : error LNK2019: unresolved external symbol php_gd_libgdimageptr_from_zval_p referenced in function zim_ffmpeg_frame_resize@@16
N:\php-sdk\php80dev\x64\Release\php_ffmpeg.dll : fatal error LNK1120: 1 unresolved externals
NMAKE : fatal error U1077: '"C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.26.28801\bin\HostX64\x64\link.exe"' : return code '0x460'
Stop.

Is there something extra that I have to do to use the PHP_GD_API function php_gd_libgdimageptr_from_zval_p?

Jan-E · 2020-07-11T02:09:44Z

I tried including php_gd.h in my config.w32 but that did not help either:

Enabling extension ext\ffmpeg [shared]
Checking for ext/gd ...  ext\gd
Checking for gd.h ...  ext\gd\libgd
Checking for gd_io.h ...  ext\gd\libgd
Checking for php_gd.h ...  ext\gd
ffmpeg-php gd support enabled; headers found at ext\gd and ext\gd\libgd

Jan-E · 2020-07-11T03:24:04Z

I could tweak config.w32 to link php_gd2.lib, but this certainly could not be the desired solution:

			var build_dir = get_define("BUILD_DIR");
			ADD_FLAG("LDFLAGS_FFMPEG", ' php_gd2.lib /libpath:"' + build_dir + '"');

Jan-E · 2020-07-11T03:46:53Z

ADD_EXTENSION_DEP("ffmpeg", "gd", true); is not the solution either. It fails with

NMAKE : fatal error U1073: don't know how to make 'N:\php-sdk\php80dev\x64\Release\php_gd.lib'

ADD_EXTENSION_DEP("ffmpeg", "gd2", true); leads to this message in the configure output:

	gd2 not found: gd2 support in ffmpeg disabled

and php_gd2.lib is not added to the link statement.

Jan-E · 2020-07-11T11:18:36Z

@cmb69 @nikic
To summarize the only way I could find to use php_gd_libgdimageptr_from_zval_p was to use a rather awkward way to link php_gd2.lib. See Jan-E/php7-ffmpeg@5187bd0

	if (PHP_VERSION > 7) {
		var build_dir = get_define("BUILD_DIR");
		ADD_FLAG("LDFLAGS_FFMPEG", ' php_gd2.lib /libpath:"' + build_dir + '"');
	}

I did not check if the same problems arise in a *nix environment.

cmb69 · 2020-07-11T12:17:33Z

Sorry, the patch is incomplete. And indeed, there is an additional issue regarding the name of the extension being "gd", but the .dll being named "php_gd2.dll" (that prevents ADD_EXTENSION_DEP()).

Jan-E · 2020-07-11T13:33:41Z

Fine with me. I am not in a hurry and happy to test. There was a remarkable difference between the linked php_gd2.lib and the copied lines. The linked php_gd2.lib version gave a warning:

ext\ffmpeg\ffmpeg_frame.c(370): warning C4047: '=': 'gdImage *' differs in levels of indirection from 'int'

Line 370 is the one at https://github.com/Jan-E/php7-ffmpeg/blob/5187bd090602c5a6741c2f0a9c54836647969e53/ffmpeg_frame.c#L370

	gd_img = php_gd_libgdimageptr_from_zval_p(return_value);

The warning should not be ignored in this case, because the toGDimage() failed once again. I reverted my last commit.

cmb69 · 2020-07-11T14:02:55Z

The linked php_gd2.lib version gave a warning:

This warning is likely the result of php_gd_libgdeclareddimageptr_from_zval_p() not having been declared, in which case the return type is assumed to be int.

There's not really much point in giving the DLL a version number, since there is no php_gd.dll for years (if there ever has been). Renaming, on the hand, matches the name on other systems (gd.so), and allows to actually use `ADD_EXTENSION_DEP()`.

cmb69 · 2020-07-11T14:56:29Z

The missing support code for importing should now be in place. I have also added a commit which renames php_gd2.dll to php_gd.dll. https://gist.github.com/cmb69/048d3a2a60fe0b5417ff70fc1af0b18b has the code I used for testing this PR.

Jan-E · 2020-07-11T17:10:35Z

Looking good. Jan-E/php7-ffmpeg@89afa67 and Jan-E/php7-ffmpeg@36be7d4
Works OK without any compiler warnings.

cmb69 · 2020-07-11T17:34:27Z

Thanks! Applied as 340e2ea.

cmb69 referenced this pull request in Jan-E/php7-ffmpeg Jul 10, 2020

Fixed ffmpeg_frame->toGDImage() /* hooray */

c7d3316

Todo: check why FFMPEG_PHP_FETCH_IMAGE_RESOURCE is still used in PHP 8

nikic reviewed Jul 10, 2020

View reviewed changes

cmb69 marked this pull request as draft July 10, 2020 10:53

Export php_gd_libgdimageptr_from_zval_p()

037e8d5

Some extension may need to retrieve the `gdImagePtr` from an `GdImage` object; thus, we export the respective function. To not being forced to include gd.h in php_gd.h, we use the opaque `struct gdImageStruct *` as return type.

cmb69 force-pushed the cmb/gdimage-export branch from 305d373 to 037e8d5 Compare July 10, 2020 17:22

cmb69 marked this pull request as ready for review July 10, 2020 21:05

cmb69 marked this pull request as draft July 11, 2020 12:17

cmb69 added 2 commits July 11, 2020 16:53

Allow to import the function from other DLLs

6103edb

Rename php_gd2.dll to php_gd.dll

dbd00e4

There's not really much point in giving the DLL a version number, since there is no php_gd.dll for years (if there ever has been). Renaming, on the hand, matches the name on other systems (gd.so), and allows to actually use `ADD_EXTENSION_DEP()`.

cmb69 marked this pull request as ready for review July 11, 2020 16:52

Jan-E referenced this pull request in Jan-E/php7-ffmpeg Jul 11, 2020

include <php_gd.h>, switch from gdImage to gdImagePtr

89afa67

nikic approved these changes Jul 11, 2020

View reviewed changes

cmb69 closed this Jul 11, 2020

cmb69 deleted the cmb/gdimage-export branch July 11, 2020 17:34

carusogabriel added this to the PHP 8.0 milestone Jul 11, 2020

Conversation

cmb69 commented Jul 9, 2020

Uh oh!

nikic Jul 10, 2020

Choose a reason for hiding this comment

Uh oh!

nikic Jul 10, 2020

Choose a reason for hiding this comment

Uh oh!

cmb69 Jul 10, 2020

Choose a reason for hiding this comment

Uh oh!

nikic Jul 10, 2020

Choose a reason for hiding this comment

Uh oh!

nikic Jul 10, 2020

Choose a reason for hiding this comment

Uh oh!

cmb69 Jul 10, 2020

Choose a reason for hiding this comment

Uh oh!

Jan-E commented Jul 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jan-E commented Jul 11, 2020

Uh oh!

Jan-E commented Jul 11, 2020

Uh oh!

Jan-E commented Jul 11, 2020

Uh oh!

Jan-E commented Jul 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmb69 commented Jul 11, 2020

Uh oh!

Jan-E commented Jul 11, 2020

Uh oh!

cmb69 commented Jul 11, 2020

Uh oh!

cmb69 commented Jul 11, 2020

Uh oh!

Jan-E commented Jul 11, 2020

Uh oh!

cmb69 commented Jul 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Jan-E commented Jul 11, 2020 •

edited

Loading

Jan-E commented Jul 11, 2020 •

edited

Loading