sizeofint / webpanimation

golang webp animation
BSD 3-Clause "New" or "Revised" License
19 stars 4 forks source link

Are you interested in these commits? #1

Closed Benau closed 3 years ago

Benau commented 3 years ago

https://github.com/Benau/webpanimation/commit/54b987b285ffb0a735622cd392f549fd7c7c8b50 so something like this can be done (im go noob btw, you can tell if it can be done in other way)

type towebp struct {
    timestamp int
    webpanim *webpanimation.WebpAnimation
    config *webpanimation.WebPConfig
}

func(to_webp *towebp) init(w uint, h uint) {
    to_webp.timestamp = 0
    to_webp.webpanim = webpanimation.NewWebpAnimation(int(w), int(h), 0)
    to_webp.config = webpanimation.NewWebpConfig()
}

func(to_webp *towebp) SupportsAnimation() bool {
    return true
}

func (to_webp *towebp) AddFrame(image *image.RGBA, fps float32) error {
    err := to_webp.webpanim.AddFrameRGBA(image, to_webp.timestamp, to_webp.config)
    to_webp.timestamp += int((1.0 / fps) * 1000.)
    return err
}

func (to_webp *towebp) Result() []byte {
    var buf bytes.Buffer
    err := to_webp.webpanim.Encode(&buf)
    if err != nil {
        return nil
    }
    to_webp.webpanim.ReleaseMemory()
    return buf.Bytes()
}

https://github.com/Benau/webpanimation/commit/95632c1149381305312b874b3230dd33c17d08c6 avoid unneeded copying if input image is already rgba

then i don't need to maintain another fork...

sizeofint commented 3 years ago

Hi, thanks for opening this issue and taking your time to troubleshoot this. I saw your solution but I think there is better way to do this, I mean without adding new method, we can simply check type before converting to RGBA I added new branch called rgba https://github.com/sizeofint/webpanimation/tree/rgba this should sattisfy the perfomance issue if it is already RGBA type after some testing I will merge rgba to master

Benau commented 3 years ago

https://github.com/Benau/webpanimation/commit/aa4292d26384bed014e79f03dc425d3155ca4a68 You may find this interesting too, because if some project go mod vendor your project it will fail to compile because ./internal/* is not bundled

Benau commented 3 years ago

and your rgba commit works for me too

Benau commented 3 years ago

and in that commit i updated to libwebp-1.2 btw

sizeofint commented 3 years ago

Yeah, vendoring c sources (with go mod vendor command) is long time issue in golang when it comes to original sources placed in subdirectories, more info about this issue: https://github.com/golang/go/issues/26366 The only way to solve that issue is to embed all c source next to go files in project folder like you did... If you make pull request on rgba branch I would merge your solution, but there is one thing... you changed webPConfig to WebPConfig this could make problems, the reason I left that variable as private is that, it needs initialization and should be retrieved via webpanimation.NewWebpConfig method

Benau commented 3 years ago

Can you come up with an optimal solution for people like me need to store webp* stuff to a encoder structure?

Maybe use a generic interface WebConfig, and private structure of webPConfig with Set/Getter?

Benau commented 3 years ago

How about this for WebPConfig: https://github.com/Benau/webpanimation/commit/087477ded75cb46185d29096d6c5cfa85afbc2ba

And this for WebPAnimation: https://github.com/Benau/webpanimation/commit/9a748c6a610a550213a34cfdc9b61031de6075a5

Or if you want this too (May need to export more to make examples working):

diff --git a/webpanimation.go b/webpanimation.go
index bf54e32..2502aa4 100644
--- a/webpanimation.go
+++ b/webpanimation.go
@@ -8,6 +8,15 @@ import (
        "io"
 )

+type WebpAnimation interface {
+       // ReleaseMemory release memory
+       ReleaseMemory()
+       // AddFrame add frame to animation
+       AddFrame(img image.Image, timestamp int, webcfg WebPConfig) error
+       // Encode encode animation
+       Encode(w io.Writer) error
+}
+
 type webpAnimation struct {
        WebPAnimEncoderOptions *WebPAnimEncoderOptions
        Width                  int
@@ -20,7 +29,7 @@ type webpAnimation struct {
 }

 // NewWebpAnimation Initialize animation
-func NewWebpAnimation(width, height, loopCount int) *webpAnimation {
+func NewWebpAnimation(width, height, loopCount int) WebpAnimation {
        webpAnimation := &webpAnimation{loopCount: loopCount, Width: width, Height: height}
        webpAnimation.WebPAnimEncoderOptions = &WebPAnimEncoderOptions{}

@@ -30,7 +39,6 @@ func NewWebpAnimation(width, height, loopCount int) *webpAnimation {
        return webpAnimation
 }

-// ReleaseMemory release memory
 func (wpa *webpAnimation) ReleaseMemory() {
        WebPDataClear(wpa.WebPData)
        WebPMuxDelete(wpa.WebPMux)
@@ -40,7 +48,6 @@ func (wpa *webpAnimation) ReleaseMemory() {
        WebPAnimEncoderDelete(wpa.AnimationEncoder)
 }

-// AddFrame add frame to animation
 func (wpa *webpAnimation) AddFrame(img image.Image, timestamp int, webpcfg WebPConfig) error {
        var webPPicture *WebPPicture = nil
        var m *image.RGBA
@@ -71,7 +78,6 @@ func (wpa *webpAnimation) AddFrame(img image.Image, timestamp int, webpcfg WebPC
        return nil
 }

-// Encode encode animation
 func (wpa *webpAnimation) Encode(w io.Writer) error {
        wpa.WebPData = &WebPData{}
sizeofint commented 3 years ago

I tested tmp branch and examples are working, and your solution looks right, you can make pull request of tmp branch on master and I will merge it