From 2306977c831608f9330857f880667436e059de46 Mon Sep 17 00:00:00 2001 From: Dan Persa Date: Thu, 1 Nov 2018 16:39:16 +0100 Subject: [PATCH] Avoid calling the image transformation multiple times; Fixes #119 --- README.md | 7 ++- dataclient/dataclient.go | 12 ++--- dataclient/dataclient_test.go | 13 ----- eskip/sample.eskip | 7 ++- filters/crop.go | 1 + filters/finalizeresponse.go | 6 ++- filters/finalizeresponse_test.go | 3 +- filters/imagefilter.go | 50 ++++++++++++------- filters/imagefilter_test.go | 3 +- ...eryParams.go => transformbyqueryparams.go} | 42 ++++------------ ...test.go => transformbyqueryparams_test.go} | 2 +- packaging/build.sh | 2 +- 12 files changed, 64 insertions(+), 84 deletions(-) rename filters/{transformFromQueryParams.go => transformbyqueryparams.go} (76%) rename filters/{transformFromQueryParams_test.go => transformbyqueryparams_test.go} (97%) diff --git a/README.md b/README.md index dd52488..c050598 100644 --- a/README.md +++ b/README.md @@ -108,16 +108,15 @@ Skrop provides a set of filters, which you can use within the routes: * **height(size, opt-enlarge)** — resizes the image to the specified height keeping the ratio. If the second arg is specified and it is equals to "DO_NOT_ENLARGE", the image will not be enlarged * **blur(sigma, min_ampl)** — blurs the image (for info see [here](http://www.vips.ecs.soton.ac.uk/supported/current/doc/html/libvips/libvips-convolution.html#vips-gaussblur)) * **imageOverlay(filename, opacity, gravity, opt-top-margin, opt-right-margin, opt-bottom-margin, opt-left-margin)** — puts an image onverlay over the required image -* **transformFromQueryParams()** - transforms the image based on the request query parameters (supports only crop for now) e.g: localhost:9090/images/S/big-ben.jpg?crop=120,300,500,300. +* **transformByQueryParams()** - transforms the image based on the request query parameters (supports only crop for now) e.g: localhost:9090/images/S/big-ben.jpg?crop=120,300,500,300. * **cropByFocalPoint(targetX, targetY, aspectRatio, minWidth)** — crops the image based on a focal point on both the source as well as on the target and desired aspect ratio of the target. TargetX and TargetY are the definition of the target image focal point defined as relative values for both width and height, i.e. if the focal point of the target image should be right in the center it would be 0.5 and 0.5. This filter expects two PathParams named **focalPointX** and **focalPointY** which are absolute X and Y coordinates of the focal point in the source image. The fourth parameter is optional; when given the filter will ensure that the resulting image has at least the specified minimum width if not it will crop the biggest possible part based on the focal point. ### About filters The eskip file defines a list of configuration. Every configuration is composed by a route and a list of filters to -apply for that specific route. Skrop adds by default two filters (setupResponse() and finalizeResponse()). -The filter setupResponse() initialize the response by adding in the context the image received from the backend. +apply for that specific route. The finalizeResponse() needs to be added at the end, because it triggers the last transformation of the image. -Because of performance, each filter does not trigger a transformation of the image, but if possible it is merged with +Because of performance, most of the filters don't not trigger a transformation of the image, but if possible it is merged with the result of the previous filters. The image is actually transformed every time the filter cannot be merged with the previous one e.g. both edit the same attribute and also at the end of the filter chain by the finalizeResponse filter. diff --git a/dataclient/dataclient.go b/dataclient/dataclient.go index b095698..24b65db 100644 --- a/dataclient/dataclient.go +++ b/dataclient/dataclient.go @@ -2,7 +2,6 @@ package dataclient import ( log "github.com/sirupsen/logrus" - "github.com/zalando-stups/skrop/filters" "github.com/zalando/skipper/eskip" "github.com/zalando/skipper/eskipfile" "github.com/zalando/skipper/routing" @@ -16,15 +15,8 @@ type skropDataClient struct { // NewSkropDataClient creates a new dataclient specific for skrop func NewSkropDataClient(eskipFile string) routing.DataClient { - emptyArgs := make([]interface{}, 0) - - pre := &eskip.Filter{ - Name: filters.FinalizeResponseName, - Args: emptyArgs, - } return skropDataClient{ fileName: eskipFile, - prepend: pre, } } @@ -41,6 +33,10 @@ func (s skropDataClient) LoadAll() ([]*eskip.Route, error) { return nil, err } + if s.prepend == nil { + return routes, nil + } + for _, route := range routes { route.Filters = append([]*eskip.Filter{s.prepend}, route.Filters...) } diff --git a/dataclient/dataclient_test.go b/dataclient/dataclient_test.go index 4be5c62..dc04959 100644 --- a/dataclient/dataclient_test.go +++ b/dataclient/dataclient_test.go @@ -10,19 +10,6 @@ func TestNewSkropDataClient(t *testing.T) { assert.NotNil(t, s) } -func TestSkropDataClient_LoadAll(t *testing.T) { - //given - s := NewSkropDataClient("eskip_test.eskip") - - //when - routes, _ := s.LoadAll() - - //then - for _, route := range routes { - assert.Equal(t, "finalizeResponse", route.Filters[0].Name) - } -} - func TestSkropDataClient_LoadAll_FileErr(t *testing.T) { //given s := NewSkropDataClient("nonexistent.eskip") diff --git a/eskip/sample.eskip b/eskip/sample.eskip index 7af4b9a..182d8f0 100644 --- a/eskip/sample.eskip +++ b/eskip/sample.eskip @@ -22,10 +22,9 @@ smallCached: Path("/cached/images/S/:image") -> "http://localhost:9090"; dynamic: Path("/images/dynamic/:image") - -> modPath("^/images/dynamic", "/images") - -> finalizeResponse() - -> transformFromQueryParams() - -> "http://localhost:9090"; + -> modPath("^/images/dynamic", "/images") + -> transformByQueryParams() + -> "http://localhost:9090"; smallAndSharp: Path("/images/Ss/:image") -> modPath("^/images/Ss", "/images") diff --git a/filters/crop.go b/filters/crop.go index 7be15dc..8fafee2 100644 --- a/filters/crop.go +++ b/filters/crop.go @@ -83,5 +83,6 @@ func (f *crop) CreateFilter(args []interface{}) (filters.Filter, error) { func (f *crop) Request(ctx filters.FilterContext) {} func (f *crop) Response(ctx filters.FilterContext) { + log.Debugf("Response %s\n", CropName) HandleImageResponse(ctx, f) } diff --git a/filters/finalizeresponse.go b/filters/finalizeresponse.go index ae97f97..5670ba4 100644 --- a/filters/finalizeresponse.go +++ b/filters/finalizeresponse.go @@ -1,6 +1,9 @@ package filters -import "github.com/zalando/skipper/filters" +import ( + log "github.com/sirupsen/logrus" + "github.com/zalando/skipper/filters" +) // This filter is the default filter for every configuration of skrop type finalizeResponse struct{} @@ -29,5 +32,6 @@ func (s *finalizeResponse) Request(ctx filters.FilterContext) {} //finalize the response calling the transformer for the image one last time before returning the image to the client func (s *finalizeResponse) Response(ctx filters.FilterContext) { + log.Debugf("Response %s", FinalizeResponseName) FinalizeResponse(ctx) } diff --git a/filters/finalizeresponse_test.go b/filters/finalizeresponse_test.go index e3e94d5..d2b91d2 100644 --- a/filters/finalizeresponse_test.go +++ b/filters/finalizeresponse_test.go @@ -1,9 +1,9 @@ package filters import ( + "github.com/danpersa/bimg" "github.com/stretchr/testify/assert" "github.com/zalando-stups/skrop/filters/imagefiltertest" - "github.com/danpersa/bimg" "io/ioutil" "testing" ) @@ -49,6 +49,7 @@ func TestFinalizeResponse_Response(t *testing.T) { bag[skropOptions] = opt bag[skropImage] = imagefiltertest.PortraitImage() bag[skropInit] = true + bag[hasMergedFilters] = true ctx := createContext(t, "GET", "url", imagefiltertest.PortraitImageFile, bag) diff --git a/filters/imagefilter.go b/filters/imagefilter.go index 9ed52fd..11a1ef5 100644 --- a/filters/imagefilter.go +++ b/filters/imagefilter.go @@ -4,10 +4,10 @@ import ( "bytes" "errors" "fmt" + "github.com/danpersa/bimg" log "github.com/sirupsen/logrus" "github.com/zalando-stups/skrop/messages" "github.com/zalando/skipper/filters" - "github.com/danpersa/bimg" "io/ioutil" "net/http" "os" @@ -26,11 +26,12 @@ const ( // Center Gravity Center = "center" // Quality used by default if not specified - Quality = 100 - doNotEnlarge = "DO_NOT_ENLARGE" - skropImage = "skImage" - skropOptions = "skOptions" - skropInit = "skInit" + Quality = 100 + doNotEnlarge = "DO_NOT_ENLARGE" + skropImage = "skImage" + hasMergedFilters = "hasMergedFilters" + skropOptions = "skOptions" + skropInit = "skInit" ) var ( @@ -54,7 +55,7 @@ func init() { Center: bimg.GravityCentre} val, exists := os.LookupEnv("STRIP_METADATA") - if (exists && strings.ToUpper(val) == "TRUE") { + if exists && strings.ToUpper(val) == "TRUE" { stripMetadata = true } } @@ -97,6 +98,8 @@ func buildParameters(ctx filters.FilterContext, image *bimg.Image) *ImageFilterC // HandleImageResponse should be called by the Response of every filter. It transforms the image func HandleImageResponse(ctx filters.FilterContext, f ImageFilter) error { + log.Debug("Handle Image Response") + //in case the response had an error from the backend or from a previous filter if ctx.Response().StatusCode > 300 { return fmt.Errorf("processing skipped, as the backend/filter reported %d status code", ctx.Response().StatusCode) @@ -106,6 +109,7 @@ func HandleImageResponse(ctx filters.FilterContext, f ImageFilter) error { if _, ok := ctx.StateBag()[skropInit]; !ok { initResponse(ctx) ctx.StateBag()[skropInit] = true + ctx.StateBag()[hasMergedFilters] = false } image, ok := ctx.StateBag()[skropImage].(*bimg.Image) @@ -116,37 +120,36 @@ func HandleImageResponse(ctx filters.FilterContext, f ImageFilter) error { return errors.New("processing failed, image not exists in the state bag") } - opt, err := f.CreateOptions(buildParameters(ctx, image)) + optionsFromRequest, err := f.CreateOptions(buildParameters(ctx, image)) if err != nil { log.Error("Failed to create options ", err.Error()) ctx.Serve(errorResponse()) return err } - opts, ok := ctx.StateBag()[skropOptions].(*bimg.Options) + optionsFromStateBag, ok := ctx.StateBag()[skropOptions].(*bimg.Options) if !ok { log.Error("context state bag does not contains the key ", skropImage) ctx.Serve(errorResponse()) return errors.New("processing failed, initialization of options not successful") } - if f.CanBeMerged(opts, opt) { - ctx.StateBag()[skropOptions] = f.Merge(opts, opt) + if f.CanBeMerged(optionsFromStateBag, optionsFromRequest) { + ctx.StateBag()[skropOptions] = f.Merge(optionsFromStateBag, optionsFromRequest) + ctx.StateBag()[hasMergedFilters] = true log.Debug("Filter ", f, " merged in ", ctx.StateBag()[skropOptions]) return nil } - //transform the image - buf, err := transformImage(image, opts) + log.Debugf("Transform the image based on the options from request: %+v", optionsFromRequest) + buf, err := transformImage(image, optionsFromRequest) if err != nil { log.Error("Failed to process image ", err.Error()) ctx.Serve(errorResponse()) return err } - // set opt in the stateBag newImage := bimg.NewImage(buf) - newOption, err := f.CreateOptions(buildParameters(ctx, newImage)) if err != nil { log.Error("Failed to create new options ", err.Error()) ctx.Serve(errorResponse()) @@ -154,14 +157,15 @@ func HandleImageResponse(ctx filters.FilterContext, f ImageFilter) error { } ctx.StateBag()[skropImage] = newImage - ctx.StateBag()[skropOptions] = newOption + ctx.StateBag()[skropOptions] = &bimg.Options{} return nil } // FinalizeResponse is called at the end of the transformations on an image to empty the queue of // operations to perform func FinalizeResponse(ctx filters.FilterContext) { - //in case the response had an error fromt he backend or from a previous filter + log.Debug("Finalize Response") + //in case the response had an error from he backend or from a previous filter if ctx.Response().StatusCode > 300 { return } @@ -173,7 +177,15 @@ func FinalizeResponse(ctx filters.FilterContext) { image := ctx.StateBag()[skropImage].(*bimg.Image) opts := ctx.StateBag()[skropOptions].(*bimg.Options) - buf, err := transformImage(image, opts) + buf := image.Image() + + var err error + + if ctx.StateBag()[hasMergedFilters] == true { + buf, err = transformImage(image, opts) + ctx.StateBag()[hasMergedFilters] = false + } + if err != nil { log.Error("failed to process image ", err.Error()) ctx.Serve(&http.Response{ @@ -193,7 +205,7 @@ func FinalizeResponse(ctx filters.FilterContext) { func transformImage(image *bimg.Image, opts *bimg.Options) ([]byte, error) { defOpt := applyDefaults(opts) - log.Debug("successfully applied the following options on the image: ", opts) + log.Debugf("successfully applied the following options on the image: %+v\n", opts) transformedImageBytes, err := image.Process(*defOpt) diff --git a/filters/imagefilter_test.go b/filters/imagefilter_test.go index 87610b6..a0a8e47 100644 --- a/filters/imagefilter_test.go +++ b/filters/imagefilter_test.go @@ -7,10 +7,10 @@ import ( "net/http" "testing" + "github.com/danpersa/bimg" "github.com/stretchr/testify/assert" "github.com/zalando-stups/skrop/filters/imagefiltertest" "github.com/zalando/skipper/filters/filtertest" - "github.com/danpersa/bimg" ) const ( @@ -125,6 +125,7 @@ func createDefaultContext(t *testing.T, url string) *filtertest.Context { bag[skropImage] = bimg.NewImage(buffer) bag[skropOptions] = &bimg.Options{} bag[skropInit] = true + bag[hasMergedFilters] = true return createContext(t, "GET", url, imagefiltertest.PNGImageFile, bag) } diff --git a/filters/transformFromQueryParams.go b/filters/transformbyqueryparams.go similarity index 76% rename from filters/transformFromQueryParams.go rename to filters/transformbyqueryparams.go index a6d33a4..4d201cc 100644 --- a/filters/transformFromQueryParams.go +++ b/filters/transformbyqueryparams.go @@ -1,16 +1,16 @@ package filters import ( - "github.com/zalando/skipper/filters" "github.com/danpersa/bimg" + log "github.com/sirupsen/logrus" + "github.com/zalando/skipper/filters" "strconv" "strings" ) const ( - ExtractArea = "transformFromQueryParams" - cropParameters = "crop" - focalPointCropParameters = "focal_point_crop" + TransformByQueryParamsName = "transformByQueryParams" + cropParameters = "crop" ) type transformFromQueryParams struct{} @@ -20,7 +20,7 @@ func NewTransformFromQueryParams() filters.Spec { } func (t *transformFromQueryParams) Name() string { - return ExtractArea + return TransformByQueryParamsName } func (t *transformFromQueryParams) CreateFilter(config []interface{}) (filters.Filter, error) { @@ -30,20 +30,8 @@ func (t *transformFromQueryParams) CreateFilter(config []interface{}) (filters.F func (t *transformFromQueryParams) CreateOptions(ctx *ImageFilterContext) (*bimg.Options, error) { // Get crop prams from the request params, ok := ctx.Parameters[cropParameters] - if !ok { - - params, ok = ctx.Parameters[focalPointCropParameters] - - if !ok { - return &bimg.Options{}, nil - } - - params = strings.Split(params[0], ",") - if len(params) != 5 { - return &bimg.Options{}, nil - } - // TODO do focal point crop + if !ok { return &bimg.Options{}, nil } @@ -89,6 +77,8 @@ func (t *transformFromQueryParams) CreateOptions(ctx *ImageFilterContext) (*bimg width = imgSize.Width - x } + log.Debugf("Crop options x=%d y=%d width=%d height=%d\n", x, y, width, height) + return &bimg.Options{ Top: y, Left: x, @@ -97,29 +87,19 @@ func (t *transformFromQueryParams) CreateOptions(ctx *ImageFilterContext) (*bimg }, nil } -func min(a, b int) int { - if a < b { - return a - } - return b -} - func (t *transformFromQueryParams) CanBeMerged(other *bimg.Options, self *bimg.Options) bool { return false } func (t *transformFromQueryParams) Merge(other *bimg.Options, self *bimg.Options) *bimg.Options { - other.AreaWidth = self.AreaWidth - other.AreaHeight = self.AreaHeight - other.Top = self.Top - other.Left = self.Left - return other + return nil } func (t *transformFromQueryParams) Request(ctx filters.FilterContext) { - } func (e *transformFromQueryParams) Response(ctx filters.FilterContext) { + log.Debugf("Response %s\n", TransformByQueryParamsName) HandleImageResponse(ctx, e) + FinalizeResponse(ctx) } diff --git a/filters/transformFromQueryParams_test.go b/filters/transformbyqueryparams_test.go similarity index 97% rename from filters/transformFromQueryParams_test.go rename to filters/transformbyqueryparams_test.go index 42c544f..0e5e635 100644 --- a/filters/transformFromQueryParams_test.go +++ b/filters/transformbyqueryparams_test.go @@ -8,7 +8,7 @@ import ( func TestNewtransformFromQueryParams(t *testing.T) { name := NewTransformFromQueryParams().Name() - assert.Equal(t, ExtractArea, name) + assert.Equal(t, TransformByQueryParamsName, name) } func TestTransformFromQueryParams_CanBeMerged(t *testing.T) { diff --git a/packaging/build.sh b/packaging/build.sh index c86faf6..af49bfe 100755 --- a/packaging/build.sh +++ b/packaging/build.sh @@ -18,7 +18,7 @@ function make_binary() { -e "GOOS=linux" \ -w /go/src/${GO_PROJECT_NAME} \ "$DOCKER_IMAGE_NAME-build" sh \ - -c 'glide install && go build ./cmd/skrop' \ + -c 'go build ./cmd/skrop' \ && return 0 }