On Thu, May 21, 2015 at 4:18 PM, Tom Butterworth <[email protected]> wrote:
>> diff --git a/Changelog b/Changelog
>> index 9d73449..f7c59e9 100644
>> --- a/Changelog
>> +++ b/Changelog
>> @@ -30,6 +30,7 @@ version <next>:
>> - Canopus HQ/HQA decoder
>> - Automatically rotate videos based on metadata in avconv
>> - improved Quickdraw compatibility
>> +- HAP decoder and encoder
>>
>
> Hap, not HAP (throughout your patch)
> Also Vidvox not VidVox or VideoVox as it appears variously
ok will amend
>> version 11:
>> diff --git a/configure b/configure
>> index 5bcc2df..80cda8e 100755
>> --- a/configure
>> +++ b/configure
>> @@ -220,6 +220,7 @@ External library support:
>> native MPEG-4/Xvid encoder exists [no]
>> --enable-mmal enable decoding via MMAL [no]
>> --enable-openssl enable openssl [no]
>> + --enable-snappy enable snappy decompression [no]
>> --enable-x11grab enable X11 grabbing (legacy) [no]
>> --enable-zlib enable zlib [autodetect]
>>
>
> Can it be autodetected?
Theoretically yes, but distributors do not like that, especially for
non system libraries.
Maybe someone will write an internal snappy decompressor so that there
won't be any external dependencies at all.
>> diff --git a/libavcodec/hapdec.c b/libavcodec/hapdec.c
>>
> <snip>
>
> +
>> +/* The first three bytes are the size of the section past the header, or
>> zero
>> + * if the length is stored in the next long word. The fourth byte in the
>> first
>> + * long word indicates the type of the current section. */
>> +static int parse_section_header(AVCodecContext *avctx)
>> +{
>> + HAPContext *ctx = avctx->priv_data;
>> + GetByteContext *gbc = &ctx->gbc;
>> + int length;
>> +
>> + if (bytestream2_get_bytes_left(gbc) < 8)
>> + return AVERROR_INVALIDDATA;
>>
>
> A valid section header can be 4 bytes, check for 4 here
ok
>> + if (ret != SNAPPY_OK) {
>> + av_log(avctx, AV_LOG_ERROR, "Snappy size error\n");
>> + return AVERROR_BUG;
>>
>
> This would most likely be due to malformed input so AVERROR_INVALIDDATA
Yeah, but _BUG is the kind of error that lavc returns when there is a
problem with external libraries.
>> +static int decompress_texture_thread(AVCodecContext *avctx, void *arg,
>> + int block_nb, int thread_nb)
>> +{
>> + HAPContext *ctx = avctx->priv_data;
>> + AVFrame *frame = arg;
>> + int x = (BLOCK_W * block_nb) % avctx->coded_width;
>> + int y = BLOCK_H * (BLOCK_W * block_nb / avctx->coded_width);
>> + uint8_t *p = frame->data[0] + x * PIXEL_SIZE + y * frame->linesize[0];
>> + const uint8_t *d = ctx->tex_data + block_nb * ctx->tex_rat;
>> +
>> + ctx->tex_fun(p, frame->linesize[0], d);
>> + return 0;
>> +}
>>
>
> This will write outside image boundaries when dimensions aren't a multiple
> of 4
Dimensions are always multiple of 4 since there is a FFALIGN for
coded_width/height in init.
>> diff --git a/libavcodec/hapenc.c b/libavcodec/hapenc.c
>>
> <snip>
>
>> +static void compress_texture(AVCodecContext *avctx, const AVFrame *f)
>> +{
>> + HAPContext *ctx = avctx->priv_data;
>> + uint8_t *out = ctx->tex_buf;
>> + int i, j;
>> +
>> + for (j = 0; j < avctx->height; j += 4) {
>> + for (i = 0; i < avctx->width; i += 4) {
>> + uint8_t *p = f->data[0] + i * PIXEL_SIZE + j * f->linesize[0];
>> + const int step = ctx->tex_fun(out, f->linesize[0], p);
>> + out += step;
>> + }
>> + }
>> +}
>>
>
> This will read outside image boundaries when dimensions aren't a multiple
> of 4.
input surfaces are always padded, so that shouldn't happen
> Additionally for such images it gives very poor quality along image edges
> because it uses garbage pixels to determine colour values.
Well it uses *black* color not garbage. During my tests, I haven't
experienced a lot of edge deterioration. Have you?
>> + /* If there is no gain from snappy, just use the raw texture */
>> + if (final_size > ctx->tex_size) {
>> + comp = COMP_NONE;
>> + av_log(avctx, AV_LOG_WARNING,
>> + "Snappy buffer bigger than uncompressed (%lu > %lu
>> bytes).\n",
>> + final_size, ctx->tex_size);
>>
>
> Is this worthy of a warning?
Not familiar with snappy enough if its common that uncompress is
better than compressed.
I can remove or reduce its level if you prefer.
>> + /* Texture compression ratio is constant, so can we computer
>> + * beforehand the final size of the uncompressed buffer */
>> + ctx->tex_size = avctx->width * avctx->height * PIXEL_SIZE / ratio;
>>
>
> You need to align the dimensions to 4 to arrive at tex_size
True, will fix
> diff --git a/libavformat/isom.c b/libavformat/isom.c
>> @@ -250,6 +250,10 @@ const AVCodecTag ff_codec_movvideo_tags[] = {
>>
>> { AV_CODEC_ID_AIC, MKTAG('i', 'c', 'o', 'd') },
>>
>> + { AV_CODEC_ID_HAP, MKTAG('H', 'A', 'P', '1') },
>> + { AV_CODEC_ID_HAP, MKTAG('H', 'A', 'P', '5') },
>> + { AV_CODEC_ID_HAP, MKTAG('H', 'A', 'P', 'Y') },
>> +
>> { AV_CODEC_ID_NONE, 0 },
>> };
>>
>
> I know these are compared internally without consideration of case, but
> Hap1, Hap5, HapY
ok
thanks for your review
--
Vittorio
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
