Thanks for the detailed review to this patch set Vittorio.
>> This change will reject frames with a texture type which doesn't match the
>> stream description.
>
> Is this change required by the format specification or just as a
> precaution for malformed streams?
Just as a precaution for malformed streams. With the change in this
patch, not rejecting frames would result in feeding bad data to the
subsequent DXT decode stage.
>
>> ---
>> libavcodec/hapdec.c | 27 +++++++++++++++++++++------
>> 1 file changed, 21 insertions(+), 6 deletions(-)
>>
>> diff --git a/libavcodec/hapdec.c b/libavcodec/hapdec.c
>> index ca1c158..324a43a 100644
>> --- a/libavcodec/hapdec.c
>> +++ b/libavcodec/hapdec.c
>> @@ -78,20 +78,19 @@ static int setup_texture(AVCodecContext *avctx, size_t
>> length)
>> const char *compressorstr;
>> int ret;
>>
>> + if ((avctx->codec_tag == MKTAG('H','a','p','1') && (ctx->section_type &
>> 0x0F) != HAP_FMT_RGBDXT1)
>> + || (avctx->codec_tag == MKTAG('H','a','p','5') &&
>> (ctx->section_type & 0x0F) != HAP_FMT_RGBADXT5)
>> + || (avctx->codec_tag == MKTAG('H','a','p','Y') &&
>> (ctx->section_type & 0x0F) != HAP_FMT_YCOCGDXT5))
>> + return AVERROR_INVALIDDATA;
>
> can you please add an error message so that the user knows what went wrong?
> style nit: please keep || at the end of the line, and the () around
> conditions can be omitted
okay
>> switch (ctx->section_type & 0x0F) {
>> case HAP_FMT_RGBDXT1:
>> - ctx->tex_rat = 8;
>> - ctx->tex_fun = ctx->dxtc.dxt1_block;
>> texture_name = "DXT1";
>> break;
>> case HAP_FMT_RGBADXT5:
>> - ctx->tex_rat = 16;
>> - ctx->tex_fun = ctx->dxtc.dxt5_block;
>> texture_name = "DXT5";
>> break;
>> case HAP_FMT_YCOCGDXT5:
>> - ctx->tex_rat = 16;
>> - ctx->tex_fun = ctx->dxtc.dxt5ys_block;
>> texture_name = "DXT5-YCoCg-scaled";
>> break;
>> default:
>> @@ -211,6 +210,22 @@ static av_cold int hap_init(AVCodecContext *avctx)
>>
>> ff_texturedsp_init(&ctx->dxtc);
>>
>> + switch (avctx->codec_tag) {
>> + case MKTAG('H','a','p','1'):
>> + ctx->tex_rat = 8;
>> + ctx->tex_fun = ctx->dxtc.dxt1_block;
>> + break;
>> + case MKTAG('H','a','p','5'):
>> + ctx->tex_rat = 16;
>> + ctx->tex_fun = ctx->dxtc.dxt5_block;
>> + break;
>> + case MKTAG('H','a','p','Y'):
>> + ctx->tex_rat = 16;
>> + ctx->tex_fun = ctx->dxtc.dxt5ys_block;
>> + break;
>> + default:
>> + return AVERROR_DECODER_NOT_FOUND;
>
> Is this change necessary? There are now two switches that carry out
> the same purpose and the condition above forces cases from both
> switches to perform the same operation.
Define "necessary" :)
Setting the tex_rat and tex_fun members to the same value every frame
is redundant. This change sets them once per session rather than every
single frame.
I considered removing the per-frame switch (which now only sets the
string for the debug log) entirely as this information can be derived
from the four-cc printed in the stream information. Maybe this patch
should indeed remove the per-frame switch and only print the
second-stage compressor (Snappy or none) per-frame?
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
