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

[Gwenole Beauchesne]

Le 29 mars 09 ? 04:28, Michael Niedermayer a ?crit :

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

I am only trying to reconstruct the original intra_dc_vlc_thr syntax  
element in the bitstream. And AFAICS, this is only defined if not  
binary shape.

That was to save an exra field from MpegEncContext since we don't  
really have an H263Context to add whatever we want for H.263.

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

What do you mean?
- Use mb_height/mb_width in get_picture_format()? That can't work IMHO.
- I don't think there is a specific field for num_macroblock_in_gob;  
rather lavc is simply decoding whatever comes until the next  
macroblock number is beyond mb_width*mb_height.

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

OK, I will drop the comment. The MPEG-4 spec indeed only has this as  
"fixed" to 1 (Table 6-24).

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

Reconstructing from s->sprite_offset/sprite_delta looks hard, so I'd  
just add new uin16_t h263_sprite_trajectory[4][2] in MpegEncContext, OK?

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

pic_param->TRD = s->picture_structure != PICT_FRAME ? s- 
 >pp_field_time : 0;
pic_param->TRB = s->picture_structure != PICT_FRAME ? s- 
 >pb_field_time : 0;
?

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

If there is none (e.g. for the initial frames), I believe the HW would  
expect 0xffffffff.