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

Michael Niedermayer michaelni
Sun Mar 29 04:28:56 CEST 2009


On Fri, Mar 27, 2009 at 09:50:32AM +0100, Gwenole Beauchesne wrote:
> On Fri, 27 Mar 2009, Michael Niedermayer wrote:
>
>> On Thu, Mar 26, 2009 at 11:44:50PM +0100, Gwenole Beauchesne wrote:
>>> Le 26 mars 09 ? 22:36, Michael Niedermayer a ?crit :
>>>
>>>> On Thu, Mar 26, 2009 at 07:32:30PM +0100, Gwenole Beauchesne wrote:
>>>>> Le 26 mars 09 ? 19:17, Michael Niedermayer a ?crit :
>>>>>
>>>>>> [...]
>>>>>>> +/** 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
>>>>>
>>>>> Then please approve
>>>>> <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2009-March/066392.html>
>>>>> ;-)
>>>>> I don't see how I can do otherwise.
>>>>
>>>> i cant approve it because it contains the same unacceptable 1 line
>>>> functions
>>>
>>> Why? And, what do you suggest then?
>>
>> just write func1() instead of
>>
>> func2(){
>>    func1();
>> }
>>
>> func2();
>>
>> why? because the code is totally unreadable as is
>
> Ah? Here is a new patch then...

[...]
> index 0000000..82937ad
> --- /dev/null
> +++ b/libavcodec/vaapi_mpeg4.c
> @@ -0,0 +1,185 @@
> +/*
> + * MPEG-4 / H.263 HW decode acceleration through VA API
> + *
> + * Copyright (C) 2008-2009 Splitted-Desktop Systems
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include "vaapi_internal.h"
> +
> +/** Reconstruct bitstream intra_dc_vlc_thr */
> +static int mpeg4_get_intra_dc_vlc_thr(MpegEncContext *s)
> +{
> +    int v = 0;

> +    if (s->shape != 2) { /* video_object_layer_shape != "binary only" */

binary only shaps work?


> +        switch (s->intra_dc_threshold) {
> +        case 99: v = 0; break;
> +        case 13: v = 1; break;
> +        case 15: v = 2; break;
> +        case 17: v = 3; break;
> +        case 19: v = 4; break;
> +        case 21: v = 5; break;
> +        case 23: v = 6; break;
> +        case 0:  v = 7; break;
> +        }
> +    }
> +    return v;
> +}
> +

> +static int vaapi_mpeg4_start_frame(AVCodecContext *avctx, av_unused const uint8_t *buffer, av_unused uint32_t size)
> +{
> +    MpegEncContext * const s = avctx->priv_data;
> +    struct vaapi_context * const vactx = avctx->hwaccel_context;
> +    VAPictureParameterBufferMPEG4 *pic_param;
> +    VAIQMatrixBufferMPEG4 *iq_matrix;
> +    int i;
> +

> +    /* Parameters defined by source_format field (Table 6-25) */
> +    const int source_format = h263_get_picture_format(s->width, s->height);
> +    static const uint16_t num_macroblocks_in_gob[8] =
> +        { 0, 8, 11, 22, 88, 352, 0, 0 };
> +    static const uint8_t num_gobs_in_vop[8] =
> +        { 0, 6, 9, 18, 18, 18, 0, 0 };

use mb_height/mb_width please or maybe we have a more fitting field
i dont remember


> +
> +    dprintf(avctx, "vaapi_mpeg4_start_frame()\n");
> +
> +    vactx->pic_param_size   = sizeof(VAPictureParameterBufferMPEG4);
> +    vactx->iq_matrix_size   = sizeof(VAIQMatrixBufferMPEG4);
> +    vactx->slice_param_size = sizeof(VASliceParameterBufferMPEG4);
> +
> +    /* Fill in VAPictureParameterBufferMPEG4 */
> +    pic_param = &vactx->pic_param.mpeg4;
> +    pic_param->vop_width                                = s->width;
> +    pic_param->vop_height                               = s->height;
> +    pic_param->forward_reference_picture                = 0xffffffff;
> +    pic_param->backward_reference_picture               = 0xffffffff;
> +    pic_param->vol_fields.value                         = 0; /* reset all bits */
> +    pic_param->vol_fields.bits.short_video_header       = avctx->codec->id == CODEC_ID_H263;
> +    pic_param->vol_fields.bits.chroma_format            = CHROMA_420;
> +    pic_param->vol_fields.bits.interlaced               = !s->progressive_sequence;

> +    pic_param->vol_fields.bits.obmc_disable             = 1; /* XXX: no support in FFmpeg */

no you got the comment wrong, there is no OBMC support in the mpeg4 spec
ffmpeg supports OBMC (with h263+)


> +    pic_param->vol_fields.bits.sprite_enable            = s->vol_sprite_usage;
> +    pic_param->vol_fields.bits.sprite_warping_accuracy  = s->sprite_warping_accuracy;
> +    pic_param->vol_fields.bits.quant_type               = s->mpeg_quant;
> +    pic_param->vol_fields.bits.quarter_sample           = s->quarter_sample;
> +    pic_param->vol_fields.bits.data_partitioned         = s->data_partitioning;
> +    pic_param->vol_fields.bits.reversible_vlc           = s->rvlc;
> +    pic_param->no_of_sprite_warping_points              = s->num_sprite_warping_points;

> +    memset(&pic_param->sprite_trajectory_du[0], 0, sizeof(pic_param->sprite_trajectory_du)); /* XXX: fill */
> +    memset(&pic_param->sprite_trajectory_dv[0], 0, sizeof(pic_param->sprite_trajectory_dv)); /* XXX: fill */

this is not going to work.


> +    pic_param->quant_precision                          = s->quant_precision;
> +    pic_param->vop_fields.value                         = 0; /* reset all bits */
> +    pic_param->vop_fields.bits.vop_coding_type          = s->pict_type - FF_I_TYPE;
> +    pic_param->vop_fields.bits.backward_reference_vop_coding_type = s->pict_type == FF_B_TYPE ? s->next_picture.pict_type - FF_I_TYPE : 0;
> +    pic_param->vop_fields.bits.vop_rounding_type        = s->no_rounding;
> +    pic_param->vop_fields.bits.intra_dc_vlc_thr         = mpeg4_get_intra_dc_vlc_thr(s);
> +    pic_param->vop_fields.bits.top_field_first          = s->top_field_first;
> +    pic_param->vop_fields.bits.alternate_vertical_scan_flag = s->alternate_scan;
> +    pic_param->vop_fcode_forward                        = s->f_code;
> +    pic_param->vop_fcode_backward                       = s->b_code;
> +    pic_param->num_gobs_in_vop                          = num_gobs_in_vop[source_format];
> +    pic_param->num_macroblocks_in_gob                   = num_macroblocks_in_gob[source_format];

> +    pic_param->TRB                                      = 0; /* XXX: fill */
> +    pic_param->TRD                                      = 0; /* XXX: fill */

i can only guess what TRB/TRD are supposed to be but 0 is likely not
correct


> +
> +    switch (s->pict_type) {
> +    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;
> +    }

id write 
if( B) X
if(!I) Y
(your code will fail for GMC ...)
besides this, are the if() needed at all? cant that stuff always
be done?


[...]
> +static int vaapi_mpeg4_decode_slice(AVCodecContext *avctx, const uint8_t *buffer, uint32_t size)
> +{
> +    MpegEncContext * const s = avctx->priv_data;
> +    VASliceParameterBufferMPEG4 *slice_param;
> +
> +    dprintf(avctx, "vaapi_mpeg4_decode_slice(): buffer %p, size %d\n", buffer, size);
> +

> +    /* video_plane_with_short_video_header() contains all GOBs in-order,
> +       and this is what VA API (Intel backend) expects: only a single
> +       slice param. So fake macroblock_number for FFmpeg so that we
> +       don't call vaapi_mpeg4_decode_slice() again */
> +    if (avctx->codec->id == CODEC_ID_H263)
> +        size = s->gb.buffer_end - buffer;

Please seperate comments a little from the code, this one felt for a
second or 2 hard to read
i mean something like:

 /* video_plane_with_short_video_header() contains all GOBs in-order,
  * and this is what VA API (Intel backend) expects: only a single
  * slice param. So fake macroblock_number for FFmpeg so that we
  * don't call vaapi_mpeg4_decode_slice() again 
  */
 if (avctx->codec->id == CODEC_ID_H263)
     size = s->gb.buffer_end - buffer;


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

Old school: Use the lowest level language in which you can solve the problem
            conveniently.
New school: Use the highest level language in which the latest supercomputer
            can solve the problem without the user falling asleep waiting.
-------------- 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/20090329/ec2890ab/attachment.pgp>



More information about the ffmpeg-devel mailing list