[FFmpeg-devel] [patch] - fixes a few prores 4444 samples

[Jonne Ahner]

On Mon, Sep 19, 2011 at 22:13, Reimar Döffinger <Reimar.Doeffinger at gmx.de>
wrote:
> On Mon, Sep 19, 2011 at 09:01:26PM +0200, Jonne Ahner wrote:
>> @@ -168,7 +168,11 @@ static int decode_frame_header(ProresContext *ctx,
const uint8_t *buf,
>>          ctx->frame.top_field_first = ctx->frame_type == 1;
>>      }
>>
>> -    avctx->pix_fmt = PIX_FMT_YUV422P10;
>> +    if (avctx->codec_tag == MKTAG('a','p','4','h')) {
>> +        avctx->pix_fmt = PIX_FMT_YUV444P10;
>> +    } else {
>> +        avctx->pix_fmt = PIX_FMT_YUV422P10;
>> +    }
>
> codec tag is a really bad way to distinguish those.
> If there's no other way to distinguish them, e.g. at runtime,
> a different codec ID should be used (IMO).

There might be a header thingy, havent looked too hard yet sorry. Related
to that I suspect ap4c is similar.


>> +static void decode_slice_chroma444(AVCodecContext *avctx, SliceContext
*slice,
>> +                              uint8_t *dst, int dst_stride,
>> +                              const uint8_t *buf, unsigned buf_size,
>> +                              const int *qmat)
>> +{
>> +    ProresContext *ctx = avctx->priv_data;
>> +    DECLARE_ALIGNED(16, DCTELEM, blocks)[8*4*64], *block;
>
> I see that is already in the current code, but...
> I think DECLARE_ALIGNED is just wrong, it has to be LOCAL_ALIGNED_16.
> Also with 4kB size IMO it would be better if it was not on the stack at
all.

This I can only trust you on. I attempted to change as little as possible.
(since I dont quite grok the code and havent done c in a long time)


>
>> @@ -491,9 +523,15 @@ static int decode_slice_thread(AVCodecContext
*avctx, void *arg, int jobnr, int
>>          chroma_stride = pic->linesize[1] << 1;
>>      }
>>
>> -    dest_y = pic->data[0] + (slice->mb_y << 4) * luma_stride +
(slice->mb_x << 5);
>> -    dest_u = pic->data[1] + (slice->mb_y << 4) * chroma_stride +
(slice->mb_x << 4);
>> -    dest_v = pic->data[2] + (slice->mb_y << 4) * chroma_stride +
(slice->mb_x << 4);
>> +    if (avctx->pix_fmt == PIX_FMT_YUV444P10) {
>> +        dest_y = pic->data[0] + (slice->mb_y << 4) * luma_stride +
(slice->mb_x << 5);
>> +        dest_u = pic->data[1] + (slice->mb_y << 4) * chroma_stride +
(slice->mb_x << 5);
>> +        dest_v = pic->data[2] + (slice->mb_y << 4) * chroma_stride +
(slice->mb_x << 5);
>> +    } else {
>> +        dest_y = pic->data[0] + (slice->mb_y << 4) * luma_stride +
(slice->mb_x << 5);
>> +        dest_u = pic->data[1] + (slice->mb_y << 4) * chroma_stride +
(slice->mb_x << 4);
>> +        dest_v = pic->data[2] + (slice->mb_y << 4) * chroma_stride +
(slice->mb_x << 4);
>> +    }
>
> You should probably avoid the duplication, using << (5 - chroma_h_shift)
or
> such.

Good idea. (bad attempt at clarity)


>> @@ -539,12 +585,15 @@ static int decode_frame(AVCodecContext *avctx, void
*data, int *data_size,
>>      int buf_size = avpkt->size;
>>      int frame_hdr_size, pic_size;
>>
>> -    if (buf_size < 28 || buf_size != AV_RB32(buf) ||
>> -        AV_RL32(buf +  4) != AV_RL32("icpf")) {
>> +    if (buf_size < 28 || AV_RL32(buf +  4) != AV_RL32("icpf")) {
>>          av_log(avctx, AV_LOG_ERROR, "invalid frame header\n");
>>          return -1;
>>      }
>>
>> +    if (buf_size != AV_RB32(buf)) {
>> +        av_log(avctx, AV_LOG_INFO, "buf_size != frame size (difference:
%d bytes) \n", abs(AV_RB32(buf) - buf_size));
>> +    }
>
> Why? Also I don't think the abs() is really helping.

Might as well be dropped. The initial check was making it error out and stop
the decode when it could just as well
ignore it. Abs was just to get the diff (in case of more serious too few
bytes) I think it is pointless info.
(perhaps debug level?)


Are there more 444 samples to try? I tried to browse the mplayer archive but
I couldnt find any.
I am curious about ap4c


Thanks for good comments.



 _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>