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

[Gwenole Beauchesne]

Hi,

On Thu, 26 Mar 2009, Michael Niedermayer wrote:

> [...]
>> +/** 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

"mpeg4 with short video header IS h263 IIRC" quoting... you in: 
<http://lists.mplayerhq.hu/pipermail/ffmpeg-user/2007-August/010684.html>
(and this is also what the ISO specs say IIRC for forward compatibility)

Besides, looking at the spec for video_plane_with_short_header() and the 
actual implementation in FFmpeg, this is h263_decode_picture_header(). 
That function seems to be only called for CODEC_ID_H263.

Could this doxy be any better?
/** Determine short_video_header (H.263 for forward compatibility) */

>> +/** 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)

mpeg4_get_num_macroblocks_in_gob() and mpeg4_get_num_gobs_in_vop() would 
still be needed since there is no equivalent Table 6-25 in FFmpeg. The 
best approaching table is h263_format[] which is... limited to formats.

>> +/** 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

It indeed can't, unless there is a bug in the bitstream that lead FFmpeg 
to set incorrect value for intra_dc_threshold.

>> +        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

Unfortunately, it can't: not the same data structures, neither the same 
number of arrays to fill in.

New patch attached wuth the suggested changes.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ffmpeg.hwaccel.vaapi.mpeg4.6.patch
Type: text/x-diff
Size: 12155 bytes
Desc: 
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090326/50349613/attachment.patch>