[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