| Message ID | 20161108133054.90950-2-bangnoise@gmail.com |
|---|---|
| State | Accepted |
| Commit | bd6fa80d56fcda385da1c8f21eb83282a7930899 |
| Headers | show |
On Tue, Nov 08, 2016 at 01:30:54PM +0000, Tom Butterworth wrote: > The secondary compression in Hap is optional, this change exposes that option to > the user as some use-cases favour higher bitrate files to reduce workload > decoding. > Adds "none" or "snappy" as options for "compressor". Selecting "none" disregards > "chunks" option: chunking is only of benefit decompressing Snappy. > --- > libavcodec/hap.h | 1 + > libavcodec/hapenc.c | 65 +++++++++++++++++++++++++++++++++++++---------------- > 2 files changed, 47 insertions(+), 19 deletions(-) should be documented in doc/encoders.texi end of comments from me thx [...]
> > Adds "none" or "snappy" as options for "compressor". Selecting "none" > disregards > > "chunks" option: chunking is only of benefit decompressing Snappy. > > --- Maybe can be useful, to have a log, if chunk_count != 1 and compression is none, to show to the user, that the chunk count option will be override by 1. Relative to this patch, with the none ( / snappy) parameter, libsnappy is not require for the entire encoder now, only when we set compression to snappy. Maybe can be useful to have something like in the tiffenc.c, where Zlib, is required for only deflate compression). Martin
Thanks for your comments Martin - > > Adds "none" or "snappy" as options for "compressor". Selecting "none" > > disregards > > > "chunks" option: chunking is only of benefit decompressing Snappy. > > > --- > > > Maybe can be useful, to have a log, > if chunk_count != 1 and compression is none, to show to the user, that the > chunk count option will be override by 1. > The existing info log in hap_init() will print eg "6 chunks requested but 1 used." as is already done when the user's choice is overridden to divide frames evenly. Is that sufficient or do you think a reason should be displayed? Relative to this patch, with the none ( / snappy) parameter, libsnappy is > not require for the entire encoder now, > only when we set compression to snappy. > Maybe can be useful to have something like in the tiffenc.c, where Zlib, is > required for only deflate compression). > As Snappy is disabled by default, I think it would be a mistake to have the default configuration of ffmpeg encode Hap without Snappy. Skipping the Snappy compression stage is not generally desirable and it should not be the default behaviour.
> > > > Maybe can be useful, to have a log, > > if chunk_count != 1 and compression is none, to show to the user, that > the > > chunk count option will be override by 1. > > > > The existing info log in hap_init() will print eg "6 chunks requested but 1 > used." as is already done when the user's choice is overridden to divide > frames evenly. Is that sufficient or do you think a reason should be > displayed? > Yes you're right, the existing log, it's maybe enough. Like suggested by Michael, if it's documented in the encoders.texi, it can help user to know what settings have an impact on the result. > > Relative to this patch, with the none ( / snappy) parameter, libsnappy is > > not require for the entire encoder now, > > only when we set compression to snappy. > > Maybe can be useful to have something like in the tiffenc.c, where Zlib, > is > > required for only deflate compression). > > > > As Snappy is disabled by default, I think it would be a mistake to have the > default configuration of ffmpeg encode Hap without Snappy. Skipping the > Snappy compression stage is not generally desirable and it should not be > the default behaviour. > Yes, maybe don't output by default hap without snappy, it's the best choice. Thanks Martin
Thanks Michael and Martin I'll push this and offer a new patch with documentation for the Hap encoder in doc/encoders.texi. On Wed, 9 Nov 2016 at 07:37 Martin Vignali <martin.vignali@gmail.com> wrote: > > > > > > Maybe can be useful, to have a log, > > > if chunk_count != 1 and compression is none, to show to the user, that > > the > > > chunk count option will be override by 1. > > > > > > > The existing info log in hap_init() will print eg "6 chunks requested > but 1 > > used." as is already done when the user's choice is overridden to divide > > frames evenly. Is that sufficient or do you think a reason should be > > displayed? > > > > Yes you're right, the existing log, it's maybe enough. > Like suggested by Michael, if it's documented in the encoders.texi, it can > help user to know what settings have an impact on the result. > > > > > > Relative to this patch, with the none ( / snappy) parameter, libsnappy is > > > not require for the entire encoder now, > > > only when we set compression to snappy. > > > Maybe can be useful to have something like in the tiffenc.c, where > Zlib, > > is > > > required for only deflate compression). > > > > > > > As Snappy is disabled by default, I think it would be a mistake to have > the > > default configuration of ffmpeg encode Hap without Snappy. Skipping the > > Snappy compression stage is not generally desirable and it should not be > > the default behaviour. > > > > Yes, maybe don't output by default hap without snappy, it's the best > choice. > > Thanks > > Martin > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
diff --git a/libavcodec/hap.h b/libavcodec/hap.h index e4762ee..f39e621 100644 --- a/libavcodec/hap.h +++ b/libavcodec/hap.h @@ -65,6 +65,7 @@ typedef struct HapContext { enum HapTextureFormat opt_tex_fmt; /* Texture type (encoder only) */ int opt_chunk_count; /* User-requested chunk count (encoder only) */ + int opt_compressor; /* User-requested compressor (encoder only) */ int chunk_count; HapChunk *chunks; diff --git a/libavcodec/hapenc.c b/libavcodec/hapenc.c index 79b6074..3a1bc87 100644 --- a/libavcodec/hapenc.c +++ b/libavcodec/hapenc.c @@ -204,15 +204,25 @@ static int hap_encode(AVCodecContext *avctx, AVPacket *pkt, if (ret < 0) return ret; - /* DXTC compression. */ - ret = compress_texture(avctx, ctx->tex_buf, ctx->tex_size, frame); - if (ret < 0) - return ret; - - /* Compress (using Snappy) the frame */ - final_data_size = hap_compress_frame(avctx, pkt->data + header_length); - if (final_data_size < 0) - return final_data_size; + if (ctx->opt_compressor == HAP_COMP_NONE) { + /* DXTC compression directly to the packet buffer. */ + ret = compress_texture(avctx, pkt->data + header_length, pkt->size - header_length, frame); + if (ret < 0) + return ret; + + ctx->chunks[0].compressor = HAP_COMP_NONE; + final_data_size = ctx->tex_size; + } else { + /* DXTC compression. */ + ret = compress_texture(avctx, ctx->tex_buf, ctx->tex_size, frame); + if (ret < 0) + return ret; + + /* Compress (using Snappy) the frame */ + final_data_size = hap_compress_frame(avctx, pkt->data + header_length); + if (final_data_size < 0) + return final_data_size; + } /* Write header at the start. */ hap_write_frame_header(ctx, pkt->data, final_data_size + header_length); @@ -273,10 +283,30 @@ static av_cold int hap_init(AVCodecContext *avctx) ctx->tex_size = FFALIGN(avctx->width, TEXTURE_BLOCK_W) * FFALIGN(avctx->height, TEXTURE_BLOCK_H) * 4 / ratio; - /* Round the chunk count to divide evenly on DXT block edges */ - corrected_chunk_count = av_clip(ctx->opt_chunk_count, 1, HAP_MAX_CHUNKS); - while ((ctx->tex_size / (64 / ratio)) % corrected_chunk_count != 0) { - corrected_chunk_count--; + switch (ctx->opt_compressor) { + case HAP_COMP_NONE: + /* No benefit chunking uncompressed data */ + corrected_chunk_count = 1; + + ctx->max_snappy = ctx->tex_size; + ctx->tex_buf = NULL; + break; + case HAP_COMP_SNAPPY: + /* Round the chunk count to divide evenly on DXT block edges */ + corrected_chunk_count = av_clip(ctx->opt_chunk_count, 1, HAP_MAX_CHUNKS); + while ((ctx->tex_size / (64 / ratio)) % corrected_chunk_count != 0) { + corrected_chunk_count--; + } + + ctx->max_snappy = snappy_max_compressed_length(ctx->tex_size / corrected_chunk_count); + ctx->tex_buf = av_malloc(ctx->tex_size); + if (!ctx->tex_buf) { + return AVERROR(ENOMEM); + } + break; + default: + av_log(avctx, AV_LOG_ERROR, "Invalid compresor %02X\n", ctx->opt_compressor); + return AVERROR_INVALIDDATA; } if (corrected_chunk_count != ctx->opt_chunk_count) { av_log(avctx, AV_LOG_INFO, "%d chunks requested but %d used.\n", @@ -286,12 +316,6 @@ static av_cold int hap_init(AVCodecContext *avctx) if (ret != 0) return ret; - ctx->max_snappy = snappy_max_compressed_length(ctx->tex_size / corrected_chunk_count); - - ctx->tex_buf = av_malloc(ctx->tex_size); - if (!ctx->tex_buf) - return AVERROR(ENOMEM); - return 0; } @@ -312,6 +336,9 @@ static const AVOption options[] = { { "hap_alpha", "Hap Alpha (DXT5 textures)", 0, AV_OPT_TYPE_CONST, { .i64 = HAP_FMT_RGBADXT5 }, 0, 0, FLAGS, "format" }, { "hap_q", "Hap Q (DXT5-YCoCg textures)", 0, AV_OPT_TYPE_CONST, { .i64 = HAP_FMT_YCOCGDXT5 }, 0, 0, FLAGS, "format" }, { "chunks", "chunk count", OFFSET(opt_chunk_count), AV_OPT_TYPE_INT, {.i64 = 1 }, 1, HAP_MAX_CHUNKS, FLAGS, }, + { "compressor", "second-stage compressor", OFFSET(opt_compressor), AV_OPT_TYPE_INT, { .i64 = HAP_COMP_SNAPPY }, HAP_COMP_NONE, HAP_COMP_SNAPPY, FLAGS, "compressor" }, + { "none", "None", 0, AV_OPT_TYPE_CONST, { .i64 = HAP_COMP_NONE }, 0, 0, FLAGS, "compressor" }, + { "snappy", "Snappy", 0, AV_OPT_TYPE_CONST, { .i64 = HAP_COMP_SNAPPY }, 0, 0, FLAGS, "compressor" }, { NULL }, };