Image interlacing option in GD is supported #185

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
@vchrm

vchrm commented Dec 31, 2012

Fixed previous pull request: #184 .

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Dec 31, 2012

Contributor

Please also implement it for Gmagick and Imagick

Contributor

stof commented Dec 31, 2012

Please also implement it for Gmagick and Imagick

@romainneutron

This comment has been minimized.

Show comment
Hide comment
@romainneutron

romainneutron Dec 31, 2012

Collaborator

Hello @vchrm ,

thanks for having updated your PR. As @stof said, you have to implement it with other drivers.

Do not open a new pull request for this, just update this one, by pushing your commit on your feature/interlacing branch.

Collaborator

romainneutron commented Dec 31, 2012

Hello @vchrm ,

thanks for having updated your PR. As @stof said, you have to implement it with other drivers.

Do not open a new pull request for this, just update this one, by pushing your commit on your feature/interlacing branch.

@vchrm

This comment has been minimized.

Show comment
Hide comment
@vchrm

vchrm Jan 7, 2013

Kk, I'll have a look into that (right after school exams period ends).

vchrm commented Jan 7, 2013

Kk, I'll have a look into that (right after school exams period ends).

@anthonysterling

This comment has been minimized.

Show comment
Hide comment
@anthonysterling

anthonysterling Jan 30, 2013

Contributor

With regards to interlace options, Imagick and GMagick share the following constants/options.

Imagick

Imagick::INTERLACE_UNDEFINED
Imagick::INTERLACE_NO
Imagick::INTERLACE_LINE
Imagick::INTERLACE_PLANE
Imagick::INTERLACE_PARTITION

Gmagick

Gmagick::INTERLACE_UNDEFINED
Gmagick::INTERLACE_NO
Gmagick::INTERLACE_LINE
Gmagick::INTERLACE_PLANE
Gmagick::INTERLACE_PARTITION

However, GD only has the ability so set interlacing on or off. It would be nice to know which interlacing strategy GD uses so the same behaviour can be applied with Imagick and Gmagick.

Contributor

anthonysterling commented Jan 30, 2013

With regards to interlace options, Imagick and GMagick share the following constants/options.

Imagick

Imagick::INTERLACE_UNDEFINED
Imagick::INTERLACE_NO
Imagick::INTERLACE_LINE
Imagick::INTERLACE_PLANE
Imagick::INTERLACE_PARTITION

Gmagick

Gmagick::INTERLACE_UNDEFINED
Gmagick::INTERLACE_NO
Gmagick::INTERLACE_LINE
Gmagick::INTERLACE_PLANE
Gmagick::INTERLACE_PARTITION

However, GD only has the ability so set interlacing on or off. It would be nice to know which interlacing strategy GD uses so the same behaviour can be applied with Imagick and Gmagick.

@anthonysterling

This comment has been minimized.

Show comment
Hide comment
@anthonysterling

anthonysterling Jan 30, 2013

Contributor

The libgd source points to a libjpeg function called jpeg_simple_progression which lives here.

Sadly, I still cannot determine what interlacing strategy is being used.

Contributor

anthonysterling commented Jan 30, 2013

The libgd source points to a libjpeg function called jpeg_simple_progression which lives here.

Sadly, I still cannot determine what interlacing strategy is being used.

@anthonysterling

This comment has been minimized.

Show comment
Hide comment
@anthonysterling

anthonysterling Jan 30, 2013

Contributor

I wonder if $options is the right place for this? I'd think not if we can make use of the interlace variants listed above. What about a filter?

Thoughts @avalanche123 ?

Contributor

anthonysterling commented Jan 30, 2013

I wonder if $options is the right place for this? I'd think not if we can make use of the interlace variants listed above. What about a filter?

Thoughts @avalanche123 ?

@romainneutron

This comment has been minimized.

Show comment
Hide comment
@romainneutron

romainneutron Jan 30, 2013

Collaborator

IMO we could add ImageInterface::INTERLACE_* constants, map theses constants for Imagick and Gmagick, and ignore the constant use for GD (or throw an exception for unsupported interlacing methods).

Collaborator

romainneutron commented Jan 30, 2013

IMO we could add ImageInterface::INTERLACE_* constants, map theses constants for Imagick and Gmagick, and ignore the constant use for GD (or throw an exception for unsupported interlacing methods).

@anthonysterling

This comment has been minimized.

Show comment
Hide comment
@anthonysterling

anthonysterling Jan 30, 2013

Contributor

Sounds good to me @romainneutron, and add an interlace method too?

Contributor

anthonysterling commented Jan 30, 2013

Sounds good to me @romainneutron, and add an interlace method too?

@romainneutron

This comment has been minimized.

Show comment
Hide comment
@romainneutron

romainneutron Jan 30, 2013

Collaborator

yes , sure :)

Collaborator

romainneutron commented Jan 30, 2013

yes , sure :)

@anthonysterling

This comment has been minimized.

Show comment
Hide comment
@anthonysterling

anthonysterling Jan 30, 2013

Contributor

Sorry to railroad your PR @vchrm :(

I was looking to implement this for an upcoming client project, and stumbled across this PR. Feel free to merge my branch and do as you wish.

It still needs a code review, supporting tests and documentation - all of which I'm happy to help with.

Anthony.

Contributor

anthonysterling commented Jan 30, 2013

Sorry to railroad your PR @vchrm :(

I was looking to implement this for an upcoming client project, and stumbled across this PR. Feel free to merge my branch and do as you wish.

It still needs a code review, supporting tests and documentation - all of which I'm happy to help with.

Anthony.

@anthonysterling

This comment has been minimized.

Show comment
Hide comment
@anthonysterling

anthonysterling Jan 30, 2013

Contributor

FWIW, this still feels like it should be somewhere else; it adds a lot of noise to the ImageInterface.

Contributor

anthonysterling commented Jan 30, 2013

FWIW, this still feels like it should be somewhere else; it adds a lot of noise to the ImageInterface.

@vchrm

This comment has been minimized.

Show comment
Hide comment
@vchrm

vchrm Feb 3, 2013

Hi, my apologies for the delay. I really had exams until this week. No harm done, I am happy that Imagine is going to support this.
I'm looking forward to when your pull request is merged :-).
So, shouldn't we close this one?

vchrm commented Feb 3, 2013

Hi, my apologies for the delay. I really had exams until this week. No harm done, I am happy that Imagine is going to support this.
I'm looking forward to when your pull request is merged :-).
So, shouldn't we close this one?

@avalanche123

This comment has been minimized.

Show comment
Hide comment
Owner

avalanche123 commented Feb 5, 2013

Merged @anthonysterling's PR

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