[FFmpeg-devel] [PATCH][VAAPI][4/6] Add MPEG-4 / H.263 bitstream decoding (take 5)

[Michael Niedermayer]

On Sun, Mar 22, 2009 at 02:46:20AM +0100, Gwenole Beauchesne wrote:
> On Wed, 18 Mar 2009, Gwenole Beauchesne wrote:
>
>> On Fri, 6 Mar 2009, Gwenole Beauchesne wrote:
>>
>>> On Tue, 3 Mar 2009, Michael Niedermayer wrote:
>>>> On Tue, Mar 03, 2009 at 02:36:54PM +0100, Gwenole Beauchesne wrote:
>>>>> On Thu, 26 Feb 2009, Gwenole Beauchesne wrote:
>>>>>> This patch adds MPEG-4 / H.263 decoding through VA API.
>>>>> New patch attached. No real change but the split + hwaccel_data
>>>>> infrastructure.
>>>> looks ok
>>> Thanks but the common bits would be needed first. ;-)
>>> I will post a new patch once you approve the new [2/6] (common) patch.
>>
>> In the mean-time, here a new patch to cope with recent changes.
>
> New patch to cope with (applied) common parts. Remove extraneous hunks and 
> add missing one in allcodecs.c


[...]
> +/** Reconstruct bitstream short_video_header */
> +static inline int mpeg4_get_short_video_header(MpegEncContext *s)
> +{
> +    return s->avctx->codec->id == CODEC_ID_H263;
> +}

what the function does and what the doxy says is different


> +
> +/** Reconstruct bitstream source_format (for short_video_header) */
> +static inline int mpeg4_get_source_format(MpegEncContext *s)
> +{
> +    if (!mpeg4_get_short_video_header(s))
> +        return 0;
> +    return h263_get_picture_format(s->width, s->height);
> +}
> +
> +/** Reconstruct bitstream num_macroblocks_in_gob */
> +static inline int mpeg4_get_num_macroblocks_in_gob(MpegEncContext *s)
> +{
> +    static const uint16_t num_macroblocks_in_gob_table[8] =
> +        { 0, 8, 11, 22, 88, 352, 0, 0 };
> +    return num_macroblocks_in_gob_table[mpeg4_get_source_format(s)];
> +}
> +
> +/** Reconstruct bitstream num_gobs_in_vop */
> +static inline int mpeg4_get_num_gobs_in_vop(MpegEncContext *s)
> +{
> +    static const uint16_t num_gobs_in_vop_table[8] =
> +        { 0, 6, 9, 18, 18, 18, 0, 0 };
> +    return num_gobs_in_vop_table[mpeg4_get_source_format(s)];
> +}
> +
> +/** Reconstruct bitstream vop_coding_tye */
> +static inline int mpeg4_get_vop_coding_type(MpegEncContext *s)
> +{
> +    return s->pict_type - FF_I_TYPE;
> +}
> +
> +/** Compute backward_reference_vop_coding_type (7.6.7) */
> +static inline int mpeg4_get_backward_reference_vop_coding_type(MpegEncContext *s)
> +{
> +    if (s->pict_type != FF_B_TYPE)
> +        return 0;
> +    return s->next_picture.pict_type - FF_I_TYPE;
> +}

this is all really obfuscated, please get rid of all these wraper functions
and where it exist use existing functions, (the encoder might have some i dont
remember exactly)


> +
> +/** Reconstruct bitstream intra_dc_vlc_thr */
> +static int mpeg4_get_intra_dc_vlc_thr(MpegEncContext *s)
> +{
> +    if (s->shape == MPEG4_SHAPE_BINARY_ONLY) /* "binary only" */
> +        return 0;
> +    switch (s->intra_dc_threshold) {
> +    case 99: return 0;

> +    case 13: return 1;
> +    case 15: return 2;
> +    case 17: return 3;
> +    case 19: return 4;
> +    case 21: return 5;
> +    case 23: return 6;
> +    case 0:  return 7;
> +    }

> +    assert(0);
> +    return -1;

please explain how the return can be reached


[...]
> +    switch (s->pict_type) {
> +    case FF_I_TYPE:
> +        break; // no prediction from other frames
> +    case FF_B_TYPE:
> +        pic_param->backward_reference_picture = ff_vaapi_get_surface(&s->next_picture);
> +        // fall-through
> +    case FF_P_TYPE:
> +        pic_param->forward_reference_picture = ff_vaapi_get_surface(&s->last_picture);
> +        break;
> +    default:

> +        assert(0);
> +        return -1;

please remove all assert(0) return combination they are ALL rejected
either code to the best of your knowledge can not be reached (assert)
or it must be correctly handled and abort() is not correct


> +    }
> +
> +    /* Fill in VAIQMatrixBufferMPEG4 */
> +    /* Only the first inverse quantisation method uses the weighthing matrices */
> +    if (pic_param->vol_fields.bits.quant_type) {
> +        uint8_t m[64];

this array is redundant 


> +        for (i = 0; i < 64; i++)
> +            m[i] = s->dsp.idct_permutation[ff_zigzag_direct[i]];
> +
> +        iq_matrix = &vactx->iq_matrix.mpeg4;
> +        iq_matrix->load_intra_quant_mat         = 1;
> +        iq_matrix->load_non_intra_quant_mat     = 1;
> +        vactx->iq_matrix_present                = 1;
> +
> +        for (i = 0; i < 64; i++) {
> +            int n = m[i];
> +            iq_matrix->intra_quant_mat[i]       = s->intra_matrix[n];
> +            iq_matrix->non_intra_quant_mat[i]   = s->inter_matrix[n];
> +        }

i think ive seen some similar code, maybe it can be factorized


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090326/68566ec0/attachment.pgp>