Everything

php

ext/gd adding imagecompare

php.internals

David CARLIER

2 years ago

Hi,

I plan to add imagecompare to the gd extension
<https://github.com/php/php-src/pull/14877>. No problem so far however a
little discussion of the returned value

https://github.com/php/php-src/pull/14877#issuecomment-2217686123

I personally see a value of a bitmask rather than just boolean which is not
harder to handle than imagetypes
<https://www.php.net/manual/en/function.imagetypes.php>.

Let me know your thoughts.

Cheers.

Saki Takamachi

2 years ago

Hi David,

> Hi,
>
> I plan to add imagecompare to the gd extension. No problem so far however a little discussion of the returned value
>
> https://github.com/php/php-src/pull/14877#issuecomment-2217686123
>
> I personally see a value of a bitmask rather than just boolean which is not harder to handle than imagetypes.
>
> Let me know your thoughts.
>
> Cheers.

If the return value is bool, does this mean that it will be true if the comparison targets are exactly the same, and false if there is even one difference? In other words, will we no longer be able to see where the differences were?

Regards,

Saki

David CARLIER

2 years ago

Hi Saki.

On Tue, 9 Jul 2024 at 17:49, Saki Takamachi <saki@sakiot.com> wrote:

> Hi David,
>
> > Hi,
> >
> > I plan to add imagecompare to the gd extension. No problem so far
> however a little discussion of the returned value
> >
> > https://github.com/php/php-src/pull/14877#issuecomment-2217686123
> >
> > I personally see a value of a bitmask rather than just boolean which is
> not harder to handle than imagetypes.
> >
> > Let me know your thoughts.
> >
> > Cheers.
>
> If the return value is bool, does this mean that it will be true if the
> comparison targets are exactly the same, and false if there is even one
> difference? In other words, will we no longer be able to see where the
> differences were?
>
>

It seems to be the opposite regarding his proposition gdImageCompare(im1,
im2) & GD_CMP_IMAGE. Then yes you are right, we are losing the specifics.
If you want to figure out how the image 1 and image 2 differ, you need to
use the rest of the api e.g. imagecolortransparent and all that jazz ... A
bit too complex I feel.

Saki Takamachi

2 years ago

Hi David,

> It seems to be the opposite regarding his proposition gdImageCompare(im1, im2) & GD_CMP_IMAGE. Then yes you are right, we are losing the specifics. If you want to figure out how the image 1 and image 2 differ, you need to use the rest of the api e.g. imagecolortransparent and all that jazz ... A bit too complex I feel.

Thanks for the explanation.

What I think is important to note here is that there are two types of users who will use this feature:

1. Users who simply want to know if there is a match (i.e. a bool return value is sufficient)
2. Users who want to know the details of the differences

The object return type that Kamil mentioned was something I was considering as well, but I was concerned that it would be a bit verbose for type 1 users and would necessarily incur the overhead of object creation. (This is probably an unnecessary concern, as image analysis takes much longer.)

Regards,

Saki

David CARLIER

2 years ago

On Tue, 9 Jul 2024 at 18:31, Saki Takamachi <saki@sakiot.com> wrote:

> Hi David,
>
> It seems to be the opposite regarding his proposition gdImageCompare(im1,
> im2) & GD_CMP_IMAGE. Then yes you are right, we are losing the specifics.
> If you want to figure out how the image 1 and image 2 differ, you need to
> use the rest of the api e.g. imagecolortransparent and all that jazz ... A
> bit too complex I feel.
>
>
> Thanks for the explanation.
>
> What I think is important to note here is that there are two types of
> users who will use this feature:
>
> 1. Users who simply want to know if there is a match (i.e. a bool return
> value is sufficient)
>

True. Anyhow, such users can always do imagecompare($im1, $im2) &
IMG_CMP_IMAGE themselves eventually.

> 2. Users who want to know the details of the differences
>
> The object return type that Kamil mentioned was something I was
> considering as well, but I was concerned that it would be a bit verbose for
> type 1 users and would necessarily incur the overhead of object creation.
> (This is probably an unnecessary concern, as image analysis takes much
> longer.)
>

Agreed, I ll likely just commit as is sometime this week.
Cheers.

Tim Düsterhus

2 years ago

Hi

On 7/9/24 19:53, David CARLIER wrote:

> Agreed, I ll likely just commit as is sometime this week.

I hereby formally claim that this is not a simple self-contained
feature. It does require an RFC.

The addition of global `IMAGE_CMP_*` constants effectively introduces an
entirely new "namespace" within the existing list of constants and thus
I do not believe that consistency with the existing GD functionality is
a valid reason. As such I believe they should follow the
https://wiki.php.net/rfc/namespaces_in_bundled_extensions RFC and be
placed inside the `Gd` namespace - or ideally be replaced by a proper
enum or so.

Best regards
Tim Düsterhus

David CARLIER

2 years ago

Hi

On Tue, 9 Jul 2024 at 19:16, Tim Düsterhus <tim@bastelstu.be> wrote:

> Hi
>
> On 7/9/24 19:53, David CARLIER wrote:
> > Agreed, I ll likely just commit as is sometime this week.
>
> I hereby formally claim that this is not a simple self-contained
> feature. It does require an RFC.
>
> The addition of global `IMAGE_CMP_*` constants effectively introduces an
> entirely new "namespace" within the existing list of constants and thus
> I do not believe that consistency with the existing GD functionality is
> a valid reason. As such I believe they should follow the
> https://wiki.php.net/rfc/namespaces_in_bundled_extensions RFC and be
> placed inside the `Gd` namespace - or ideally be replaced by a proper
> enum or so.
>
> Best regards
> Tim Düsterhus
>

Alright, I hear you. Anyhow, there is a debate for having a full internal
version of a somewhat similar feature. We will see..

Kamil Tekiela

2 years ago

Boolean is a much more confusing value here. A bit mask is ok, but I would
prefer a better solution. Maybe a simple value object?

David CARLIER

2 years ago

Hi,

On Tue, 9 Jul 2024 at 17:59, Kamil Tekiela <tekiela246@gmail.com> wrote:

> Boolean is a much more confusing value here. A bit mask is ok, but I would
> prefer a better solution. Maybe a simple value object?
>

That could be an option too sure.

Tim Düsterhus

2 years ago

Hi

Yes, please no more global constants with names that are shortened
beyond all recognition.

On 7/9/24 18:59, Kamil Tekiela wrote:

> Boolean is a much more confusing value here. A bit mask is ok, but I would
> prefer a better solution. Maybe a simple value object?
>

If it's "simple" then it would also be unwieldy to use. To me the
obvious modern alternative would be a list of enum cases that represent
the differences, with an empty array meaning no differences. The
function should probably be renamed 'imagediff()' or similar then.

namespace Gd;

enum ImageDifference {
case Height;
case Width;
case AlphaChannel; // or: case Transparency
case Interlacing;
// ...
}

Best regards
Tim Düsterhus