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

[Michael Niedermayer]

On Thu, Mar 26, 2009 at 10:53:25AM +0100, Gwenole Beauchesne wrote:
> 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) */

there is no syntax element called short_video_header AFAIK (not checking
specs).
also if you mean H263
write codec->id == CODEC_ID_H263 and dont obfuscate it behind a function.


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

sorry?
can it or cant it, there is no unless ...
you of course must support absolutely any combination of bits, no
input that a mallicious person might create may trigger an abort()
memleak, crash or worse

(of course there will be bugs like there are in every non trivial piece
of software, but no such issues should knowingly be left there)


[...]
> +/** Determine short_video_header (H.263 for forward compatibility) */
> +static inline int mpeg4_is_short_video_header(MpegEncContext *s)
> +{
> +    return s->avctx->codec->id == CODEC_ID_H263;
> +}
> +
> +/** Reconstruct bitstream source_format (for short_video_header) */
> +static inline int mpeg4_get_source_format(MpegEncContext *s)
> +{
> +    if (!mpeg4_is_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)];
> +}

please remove these functions

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

There will always be a question for which you do not know the correct awnser.
-------------- 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/e32ffab6/attachment.pgp>