You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by myraid <gi...@git.apache.org> on 2015/12/22 02:07:09 UTC
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
GitHub user myraid opened a pull request:
https://github.com/apache/trafficserver/pull/393
[TS-4095] adding a new example plugin for converting jpeg/png to webp…
@shukitchan @bgaff can you review and merge the PR.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/myraid/trafficserver webp_transform
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/393.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #393
----
commit 842ffde4794ee81d468eef1f0bcb342b20b63c3f
Author: Sandeep Davu <sd...@soilshine-lm.corp.yahoo.com>
Date: 2015-12-22T01:04:16Z
[TS-4095] adding a new example plugin for converting jpeg/png to webp format
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Posted by myraid <gi...@git.apache.org>.
Github user myraid commented on the pull request:
https://github.com/apache/trafficserver/pull/393#issuecomment-186013102
@bgaff can you review now. Using ImageMagick makes the code much cleaner.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/393#issuecomment-168111940
@bgaff This seems useful that it ought to be more than just an example, no ? Do we have a strategy / plan for exposing stable plugins to the community, which are using the cpp APIs?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/trafficserver/pull/393
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Posted by myraid <gi...@git.apache.org>.
Github user myraid commented on the pull request:
https://github.com/apache/trafficserver/pull/393#issuecomment-166764391
@bgaff I made the suggested changes. Also added license agreement.
@shukitchan I am working on c-lang formatting and update the PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Posted by shukitchan <gi...@git.apache.org>.
Github user shukitchan commented on the pull request:
https://github.com/apache/trafficserver/pull/393#issuecomment-166739688
2 more comments
1) Please add an ASF license blurb to all new files
2) you can also run clang-format to beautify your code a bit as well. Check out here - https://cwiki.apache.org/confluence/display/TS/Coding+Style#CodingStyle-clang-formatbinaryandconfiguration . And then you can run "make clang-format" to clean up your code.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Posted by myraid <gi...@git.apache.org>.
Github user myraid commented on the pull request:
https://github.com/apache/trafficserver/pull/393#issuecomment-167018043
What should the document consist? The design of the plugin ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/393#discussion_r48644645
--- Diff: plugins/experimental/webp_transform/compress.cc ---
@@ -0,0 +1,198 @@
+/** @file
+
+ ATSCPPAPI plugin to do webp transform.
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+*/
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+
+#include <sstream>
+#include <atscppapi/Logger.h>
+#include "compress.h"
+#include "Common.h"
+
+#define MAGIC_SIZE 12;
+using std::string;
+using std::vector;
+
+
+static int
+StreamWriter(const uint8_t *data, size_t data_size, const WebPPicture *const pic)
+{
+ WebpTransform *webp_transform = static_cast<WebpTransform *>(pic->custom_ptr);
+ webp_transform->writeImage(reinterpret_cast<const char *>(data), data_size);
+ return data_size ? data_size : 1;
+}
+
+const string WebpTransform::_errors[] = {
+ "OK", "OUT_OF_MEMORY: Out of memory allocating objects", "BITSTREAM_OUT_OF_MEMORY: Out of memory re-allocating byte buffer",
+ "NULL_PARAMETER: NULL parameter passed to function", "INVALID_CONFIGURATION: configuration is invalid",
+ "BAD_DIMENSION: Bad picture dimension. Maximum width and height "
+ "allowed is 16383 pixels.",
+ "PARTITION0_OVERFLOW: Partition #0 is too big to fit 512k.\n"
+ "To reduce the size of this partition, try using less segments "
+ "with the -segments option, and eventually reduce the number of "
+ "header bits using -partition_limit. More details are available "
+ "in the manual (`man cwebp`)",
+ "PARTITION_OVERFLOW: Partition is too big to fit 16M", "BAD_WRITE: Picture writer returned an I/O error",
+ "FILE_TOO_BIG: File would be too big to fit in 4G", "USER_ABORT: encoding abort requested by user"};
+
+void
+WebpTransform::_webpMemoryWriterClear()
+{
+ if (_writer.mem != NULL) {
+ free(_writer.mem);
+ _writer.mem = NULL;
+ _writer.size = 0;
+ _writer.max_size = 0;
+ }
+}
+
+
+WebpTransform::InputFileFormat
+WebpTransform::_getImageType(std::stringstream &input_img)
+{
+ InputFileFormat format = UNSUPPORTED;
+ uint32_t magic1, magic2;
+ uint8_t buf[12];
+ input_img.read((char *)buf, 12);
+ input_img.seekg(0, input_img.beg);
+
+ magic1 = ((uint32_t)buf[0] << 24) | (buf[1] << 16) | (buf[2] << 8) | buf[3];
+ magic2 = ((uint32_t)buf[8] << 24) | (buf[9] << 16) | (buf[10] << 8) | buf[11];
+ if (magic1 == 0x89504E47U) {
+ format = PNG_;
+ } else if (magic1 >= 0xFFD8FF00U && magic1 <= 0xFFD8FFFFU) {
+ format = JPEG_;
+ } else if (magic1 == 0x52494646 && magic2 == 0x57454250) {
+ format = WEBP_;
+ }
+ return format;
+}
+
+int
+WebpTransform::_readImage(std::stringstream &input_img)
+{
+ int ok = 0;
+
+ if (_picture.width == 0 || _picture.height == 0) {
+ // If no size specified, try to decode it as PNG/JPEG (as appropriate).
+ const InputFileFormat format = _getImageType(input_img);
+ if (format == PNG_) {
+ if (!_png_dec.init(&input_img)) {
+ _png_dec.finalize();
+ return 0;
+ }
+ ok = _png_dec.readImage(&_picture, &_metadata);
+ } else if (format == JPEG_) {
+ if (!_jpeg_dec.init(&input_img)) {
+ _jpeg_dec.finalize();
+ return 0;
+ }
+ ok = _jpeg_dec.readImage(&_picture, &_metadata);
+ } else if (format == WEBP_) {
+ TS_DEBUG(TAG, "Already webp file. Nothing to be done.");
+ }
+ }
+ if (!ok)
+ TS_DEBUG(TAG, "Unsupported image format. Failed to read image.");
+
+ return ok;
+}
+
+
+void
+WebpTransform::_allocExtraInfo()
+{
+ const int mb_w = (_picture.width + 15) / 16;
+ const int mb_h = (_picture.height + 15) / 16;
+ _picture.extra_info = (uint8_t *)malloc(mb_w * mb_h * sizeof(_picture.extra_info));
--- End diff --
I'll be honest, this looks extremely weird. You're using the extra_info structure to represent the number of bits per pixel AND then you're also using it as a pointer later, why not only use it as an opaque pointer and pass in bits per pixel as an argument to this method?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Posted by mlibbey <gi...@git.apache.org>.
Github user mlibbey commented on the pull request:
https://github.com/apache/trafficserver/pull/393#issuecomment-167851931
Think my feature request would be to allow this to be a remap plugin instead of only a global one -- so it could be enabled in much more specific cases.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on the pull request:
https://github.com/apache/trafficserver/pull/393#issuecomment-186076071
Wow, yes this much cleaner by using ImageMagik. I'm :+1: with this. Anyone else have comments?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Posted by shukitchan <gi...@git.apache.org>.
Github user shukitchan commented on the pull request:
https://github.com/apache/trafficserver/pull/393#issuecomment-186080875
I am :+1: as well
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Posted by shukitchan <gi...@git.apache.org>.
Github user shukitchan commented on the pull request:
https://github.com/apache/trafficserver/pull/393#issuecomment-167850957
seems good. would be great if you can run clang-format on this
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Posted by shukitchan <gi...@git.apache.org>.
Github user shukitchan commented on the pull request:
https://github.com/apache/trafficserver/pull/393#issuecomment-167030403
Something like this - https://github.com/apache/trafficserver/blob/master/doc/admin-guide/plugins/esi.en.rst
And it should be refered in here - https://raw.githubusercontent.com/apache/trafficserver/master/doc/admin-guide/plugins/index.en.rst
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Posted by shukitchan <gi...@git.apache.org>.
Github user shukitchan commented on the pull request:
https://github.com/apache/trafficserver/pull/393#issuecomment-166470972
need some time to go through the code. But @bgaff, any objection to put it in experimental plugin instead of example/ ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/393#discussion_r48644527
--- Diff: plugins/experimental/webp_transform/compress.cc ---
@@ -0,0 +1,198 @@
+/** @file
+
+ ATSCPPAPI plugin to do webp transform.
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+*/
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+
+#include <sstream>
+#include <atscppapi/Logger.h>
+#include "compress.h"
+#include "Common.h"
+
+#define MAGIC_SIZE 12;
+using std::string;
+using std::vector;
+
+
+static int
+StreamWriter(const uint8_t *data, size_t data_size, const WebPPicture *const pic)
+{
+ WebpTransform *webp_transform = static_cast<WebpTransform *>(pic->custom_ptr);
+ webp_transform->writeImage(reinterpret_cast<const char *>(data), data_size);
+ return data_size ? data_size : 1;
+}
+
+const string WebpTransform::_errors[] = {
+ "OK", "OUT_OF_MEMORY: Out of memory allocating objects", "BITSTREAM_OUT_OF_MEMORY: Out of memory re-allocating byte buffer",
+ "NULL_PARAMETER: NULL parameter passed to function", "INVALID_CONFIGURATION: configuration is invalid",
+ "BAD_DIMENSION: Bad picture dimension. Maximum width and height "
+ "allowed is 16383 pixels.",
+ "PARTITION0_OVERFLOW: Partition #0 is too big to fit 512k.\n"
+ "To reduce the size of this partition, try using less segments "
+ "with the -segments option, and eventually reduce the number of "
+ "header bits using -partition_limit. More details are available "
+ "in the manual (`man cwebp`)",
+ "PARTITION_OVERFLOW: Partition is too big to fit 16M", "BAD_WRITE: Picture writer returned an I/O error",
+ "FILE_TOO_BIG: File would be too big to fit in 4G", "USER_ABORT: encoding abort requested by user"};
+
+void
+WebpTransform::_webpMemoryWriterClear()
+{
+ if (_writer.mem != NULL) {
+ free(_writer.mem);
+ _writer.mem = NULL;
+ _writer.size = 0;
+ _writer.max_size = 0;
+ }
+}
+
+
+WebpTransform::InputFileFormat
+WebpTransform::_getImageType(std::stringstream &input_img)
+{
+ InputFileFormat format = UNSUPPORTED;
+ uint32_t magic1, magic2;
+ uint8_t buf[12];
+ input_img.read((char *)buf, 12);
--- End diff --
Is this the same MAGIC_SIZE as above? If so it should be used for declaring the sizeof buf, and then you should use sizeof(buf) in the read.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Posted by myraid <gi...@git.apache.org>.
Github user myraid commented on the pull request:
https://github.com/apache/trafficserver/pull/393#issuecomment-166471944
@shukitchan we could easily move it to experimental. Probably have to check additional flags in Makefile.am under experimental.
Once @bgaff is Ok I can work on moving it to experimental.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/393#issuecomment-186362474
Nit picking, but instead of BUILD_WEBP_TRANSFORM_PLUGIN, wouldn't it make more sense to just say something like BUILD_HAS_IMAGEMAGICCPP or some such (look for some precedence). This way, any future uses of ImageMagic would benefit as well.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Posted by shukitchan <gi...@git.apache.org>.
Github user shukitchan commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/393#discussion_r53426210
--- Diff: doc/admin-guide/plugins/webp_transform.en.rst ---
@@ -0,0 +1,38 @@
+.. _webp_transform:
+
+WebpTransform Plugin
+****************
+
+.. Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+This plugin converts jpeg and png images and transforms them into webp format.
+All response with content-type 'image/jpeg' or 'image/png' will go through the transform.
+Content-type is changed to 'image/webp' on successful transformation.
+
+Installation
+============
+
+Add the following line to :file:`plugin.config`::
+
+ webp_transform.so
+
+
+Note
+===================
+
+This plugin only supports jpeg and png and require libjpeg > 1.9 and libpng > 1.6.16
--- End diff --
This should change now that we are using imagemagick?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/393#discussion_r48644607
--- Diff: plugins/experimental/webp_transform/compress.cc ---
@@ -0,0 +1,198 @@
+/** @file
+
+ ATSCPPAPI plugin to do webp transform.
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+*/
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+
+#include <sstream>
+#include <atscppapi/Logger.h>
+#include "compress.h"
+#include "Common.h"
+
+#define MAGIC_SIZE 12;
+using std::string;
+using std::vector;
+
+
+static int
+StreamWriter(const uint8_t *data, size_t data_size, const WebPPicture *const pic)
+{
+ WebpTransform *webp_transform = static_cast<WebpTransform *>(pic->custom_ptr);
+ webp_transform->writeImage(reinterpret_cast<const char *>(data), data_size);
+ return data_size ? data_size : 1;
+}
+
+const string WebpTransform::_errors[] = {
+ "OK", "OUT_OF_MEMORY: Out of memory allocating objects", "BITSTREAM_OUT_OF_MEMORY: Out of memory re-allocating byte buffer",
+ "NULL_PARAMETER: NULL parameter passed to function", "INVALID_CONFIGURATION: configuration is invalid",
+ "BAD_DIMENSION: Bad picture dimension. Maximum width and height "
+ "allowed is 16383 pixels.",
+ "PARTITION0_OVERFLOW: Partition #0 is too big to fit 512k.\n"
+ "To reduce the size of this partition, try using less segments "
+ "with the -segments option, and eventually reduce the number of "
+ "header bits using -partition_limit. More details are available "
+ "in the manual (`man cwebp`)",
+ "PARTITION_OVERFLOW: Partition is too big to fit 16M", "BAD_WRITE: Picture writer returned an I/O error",
+ "FILE_TOO_BIG: File would be too big to fit in 4G", "USER_ABORT: encoding abort requested by user"};
+
+void
+WebpTransform::_webpMemoryWriterClear()
+{
+ if (_writer.mem != NULL) {
+ free(_writer.mem);
+ _writer.mem = NULL;
+ _writer.size = 0;
+ _writer.max_size = 0;
+ }
+}
+
+
+WebpTransform::InputFileFormat
+WebpTransform::_getImageType(std::stringstream &input_img)
+{
+ InputFileFormat format = UNSUPPORTED;
+ uint32_t magic1, magic2;
+ uint8_t buf[12];
+ input_img.read((char *)buf, 12);
+ input_img.seekg(0, input_img.beg);
+
+ magic1 = ((uint32_t)buf[0] << 24) | (buf[1] << 16) | (buf[2] << 8) | buf[3];
+ magic2 = ((uint32_t)buf[8] << 24) | (buf[9] << 16) | (buf[10] << 8) | buf[11];
+ if (magic1 == 0x89504E47U) {
+ format = PNG_;
+ } else if (magic1 >= 0xFFD8FF00U && magic1 <= 0xFFD8FFFFU) {
+ format = JPEG_;
+ } else if (magic1 == 0x52494646 && magic2 == 0x57454250) {
+ format = WEBP_;
+ }
+ return format;
+}
+
+int
+WebpTransform::_readImage(std::stringstream &input_img)
+{
+ int ok = 0;
+
+ if (_picture.width == 0 || _picture.height == 0) {
+ // If no size specified, try to decode it as PNG/JPEG (as appropriate).
+ const InputFileFormat format = _getImageType(input_img);
+ if (format == PNG_) {
+ if (!_png_dec.init(&input_img)) {
+ _png_dec.finalize();
+ return 0;
+ }
+ ok = _png_dec.readImage(&_picture, &_metadata);
+ } else if (format == JPEG_) {
+ if (!_jpeg_dec.init(&input_img)) {
+ _jpeg_dec.finalize();
+ return 0;
+ }
+ ok = _jpeg_dec.readImage(&_picture, &_metadata);
+ } else if (format == WEBP_) {
+ TS_DEBUG(TAG, "Already webp file. Nothing to be done.");
--- End diff --
I think you should return here to avoid erroneously logging that this file type isn't supported.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on the pull request:
https://github.com/apache/trafficserver/pull/393#issuecomment-168113227
Just out of curiosity was some of this code taken as examples from the libraries you're using? Also, it seems there is a lot of image specific code in this plugin that might not need to be there, do you think a more general purpose library such as imagemagik would allow the image specific details to be abstracted away?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Posted by myraid <gi...@git.apache.org>.
Github user myraid commented on the pull request:
https://github.com/apache/trafficserver/pull/393#issuecomment-168846815
@bgaff I reused some of the code from libwebp examples and so there is a mix of C and C++. I will check if Imagemagick can be used to abstract image specific details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Posted by myraid <gi...@git.apache.org>.
Github user myraid commented on the pull request:
https://github.com/apache/trafficserver/pull/393#issuecomment-167855489
@shukitchan done with c-lang formating.
@mlibbey good idea to have a remap plugin. Will work on it once this PR is merged.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Posted by myraid <gi...@git.apache.org>.
Github user myraid commented on the pull request:
https://github.com/apache/trafficserver/pull/393#issuecomment-186383393
@zwoop @shukitchan made changes as suggested.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Posted by jrushf1239k <gi...@git.apache.org>.
Github user jrushf1239k commented on the pull request:
https://github.com/apache/trafficserver/pull/393#issuecomment-187279799
I can't build the master branch since this was added, I think that the plugins/experimental/Makefile.am needs to be changed:
- stream_editor \
- webp_transform
+ stream_editor
+ if BUILD_HAS_IMAGEMAGICKCPP
+ SUBDIRS += webp_transform
+ endif
Otherwise when I try to build master with gcc 4.4.7 I get the following errors, no Makefile in plugins/experimental/webp_transforms
$ autoreconf -vif
$ ./configure --prefix=/opt/trafficserver --with-user=ats --with-group=ats --enable-experimental-plugins
$ make
make[3]: Entering directory `/home/jrushf1239k/trafficserver/plugins/experimental/webp_transform'
make[3]: *** No rule to make target `all'. Stop.
make[3]: Leaving directory `/home/jrushf1239k/trafficserver/plugins/experimental/webp_transform'
make[2]: *** [all-recursive] Error 1
make[2]: Leaving directory `/home/jrushf1239k/trafficserver/plugins/experimental'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/home/jrushf1239k/trafficserver/plugins'
make: *** [all-recursive] Error 1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/393#discussion_r48644738
--- Diff: plugins/experimental/webp_transform/metadata.cc ---
@@ -0,0 +1,64 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include <stdlib.h>
+#include <string.h>
+
+#include "webp/types.h"
+#include "metadata.h"
+
+void
+MetadataInit(Metadata *const metadata)
+{
+ if (metadata == NULL)
+ return;
+ memset(metadata, 0, sizeof(*metadata));
+}
+
+void
+MetadataPayloadDelete(MetadataPayload *const payload)
+{
+ if (payload == NULL)
+ return;
+ free(payload->bytes);
+ payload->bytes = NULL;
+ payload->size = 0;
+}
+
+void
+MetadataFree(Metadata *const metadata)
+{
+ if (metadata == NULL)
+ return;
+ MetadataPayloadDelete(&metadata->exif);
+ MetadataPayloadDelete(&metadata->iccp);
+ MetadataPayloadDelete(&metadata->xmp);
--- End diff --
I think you're also forgetting to delete metadata itself.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Posted by myraid <gi...@git.apache.org>.
Github user myraid commented on the pull request:
https://github.com/apache/trafficserver/pull/393#issuecomment-167011245
refactored the code to follow naming conventions as suggested by @shukitchan .
please review.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/393#discussion_r48644849
--- Diff: plugins/experimental/webp_transform/compress.cc ---
@@ -0,0 +1,198 @@
+/** @file
+
+ ATSCPPAPI plugin to do webp transform.
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+*/
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+
+#include <sstream>
+#include <atscppapi/Logger.h>
+#include "compress.h"
+#include "Common.h"
+
+#define MAGIC_SIZE 12;
+using std::string;
+using std::vector;
+
+
+static int
+StreamWriter(const uint8_t *data, size_t data_size, const WebPPicture *const pic)
+{
+ WebpTransform *webp_transform = static_cast<WebpTransform *>(pic->custom_ptr);
+ webp_transform->writeImage(reinterpret_cast<const char *>(data), data_size);
+ return data_size ? data_size : 1;
+}
+
+const string WebpTransform::_errors[] = {
+ "OK", "OUT_OF_MEMORY: Out of memory allocating objects", "BITSTREAM_OUT_OF_MEMORY: Out of memory re-allocating byte buffer",
+ "NULL_PARAMETER: NULL parameter passed to function", "INVALID_CONFIGURATION: configuration is invalid",
+ "BAD_DIMENSION: Bad picture dimension. Maximum width and height "
+ "allowed is 16383 pixels.",
+ "PARTITION0_OVERFLOW: Partition #0 is too big to fit 512k.\n"
+ "To reduce the size of this partition, try using less segments "
+ "with the -segments option, and eventually reduce the number of "
+ "header bits using -partition_limit. More details are available "
+ "in the manual (`man cwebp`)",
+ "PARTITION_OVERFLOW: Partition is too big to fit 16M", "BAD_WRITE: Picture writer returned an I/O error",
+ "FILE_TOO_BIG: File would be too big to fit in 4G", "USER_ABORT: encoding abort requested by user"};
+
+void
+WebpTransform::_webpMemoryWriterClear()
+{
+ if (_writer.mem != NULL) {
+ free(_writer.mem);
+ _writer.mem = NULL;
+ _writer.size = 0;
+ _writer.max_size = 0;
+ }
+}
+
+
+WebpTransform::InputFileFormat
+WebpTransform::_getImageType(std::stringstream &input_img)
+{
+ InputFileFormat format = UNSUPPORTED;
+ uint32_t magic1, magic2;
+ uint8_t buf[12];
+ input_img.read((char *)buf, 12);
+ input_img.seekg(0, input_img.beg);
+
+ magic1 = ((uint32_t)buf[0] << 24) | (buf[1] << 16) | (buf[2] << 8) | buf[3];
+ magic2 = ((uint32_t)buf[8] << 24) | (buf[9] << 16) | (buf[10] << 8) | buf[11];
+ if (magic1 == 0x89504E47U) {
+ format = PNG_;
+ } else if (magic1 >= 0xFFD8FF00U && magic1 <= 0xFFD8FFFFU) {
+ format = JPEG_;
+ } else if (magic1 == 0x52494646 && magic2 == 0x57454250) {
+ format = WEBP_;
+ }
+ return format;
+}
+
+int
+WebpTransform::_readImage(std::stringstream &input_img)
+{
+ int ok = 0;
+
+ if (_picture.width == 0 || _picture.height == 0) {
+ // If no size specified, try to decode it as PNG/JPEG (as appropriate).
+ const InputFileFormat format = _getImageType(input_img);
+ if (format == PNG_) {
+ if (!_png_dec.init(&input_img)) {
+ _png_dec.finalize();
+ return 0;
+ }
+ ok = _png_dec.readImage(&_picture, &_metadata);
+ } else if (format == JPEG_) {
+ if (!_jpeg_dec.init(&input_img)) {
+ _jpeg_dec.finalize();
+ return 0;
+ }
+ ok = _jpeg_dec.readImage(&_picture, &_metadata);
+ } else if (format == WEBP_) {
+ TS_DEBUG(TAG, "Already webp file. Nothing to be done.");
+ }
+ }
+ if (!ok)
+ TS_DEBUG(TAG, "Unsupported image format. Failed to read image.");
+
+ return ok;
+}
+
+
+void
+WebpTransform::_allocExtraInfo()
+{
+ const int mb_w = (_picture.width + 15) / 16;
+ const int mb_h = (_picture.height + 15) / 16;
+ _picture.extra_info = (uint8_t *)malloc(mb_w * mb_h * sizeof(_picture.extra_info));
+}
+
+bool
+WebpTransform::init()
+{
+ MetadataInit(&_metadata);
--- End diff --
Why not just use constructors to do this? All it's doing is memsetting three child structs data and size fields, it might be cleaner to just allow a consturctor to set these fields to 0 (the ones inside MetadataPayload). This code tends to mix C and C/C++, I think consistency would be preferred in example plugins.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Posted by shukitchan <gi...@git.apache.org>.
Github user shukitchan commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/393#discussion_r48389749
--- Diff: lib/atscppapi/examples/webp_transform/README ---
@@ -0,0 +1,6 @@
+onverting jpeg and png to webp
--- End diff --
small typo here? Converting ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/393#discussion_r48644987
--- Diff: plugins/experimental/webp_transform/metadata.h ---
@@ -0,0 +1,52 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#ifndef METADATA_H_
+#define METADATA_H_
+
+#include "webp/types.h"
+
+#ifdef __cplusplus
+extern "C" {
--- End diff --
Why do these need to be ```extern "C"```? By doing that you're preventing yourself from being able to use a constructor on MetadataPayload. I think instead if your image library API requires C methods then you should use a void pointer but keep your internal data class as C++ classes. Just a thought.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/393#discussion_r53425864
--- Diff: plugins/experimental/webp_transform/ImageTransform.cc ---
@@ -0,0 +1,109 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include <sstream>
+#include <iostream>
+#include <atscppapi/PluginInit.h>
+#include <atscppapi/GlobalPlugin.h>
+#include <atscppapi/TransformationPlugin.h>
+#include <atscppapi/Logger.h>
+
+#include <Magick++.h>
+
+using std::string;
+using namespace Magick;
+using namespace atscppapi;
+
+namespace
+{
+#define TAG "webp_transform"
+}
+
+class ImageTransform : public TransformationPlugin
+{
+public:
+ ImageTransform(Transaction &transaction) : TransformationPlugin(transaction, TransformationPlugin::RESPONSE_TRANSFORMATION)
+ {
+ TransformationPlugin::registerHook(HOOK_READ_RESPONSE_HEADERS);
+ InitializeMagick("");
--- End diff --
Does this need to happen for every request that is transformed?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Posted by shukitchan <gi...@git.apache.org>.
Github user shukitchan commented on the pull request:
https://github.com/apache/trafficserver/pull/393#issuecomment-167015374
Also we should write a document if we are putting this into experimental plugin directory
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---