Since this is a documentation patch and functionally changes nothing (almost), I don't have any crucial point to make. But still I have some concerns.
Please see below. ----- Original Message ----- > From: Luca Barbato <[email protected]> > To: [email protected] > Cc: > Sent: Saturday, November 26, 2011 8:35 PM > Subject: [libav-devel] [PATCH] vc1: use an enum for Frame Coding Mode > > Document it a little and possibly fix a bug in dxva2_vc1 > --- > libavcodec/dxva2_vc1.c | 4 ++-- > libavcodec/vc1.c | 16 ++++++++-------- > libavcodec/vc1.h | 12 +++++++++++- > libavcodec/vc1dec.c | 27 ++++++++++++++------------- > 4 files changed, 35 insertions(+), 24 deletions(-) > > diff --git a/libavcodec/dxva2_vc1.c b/libavcodec/dxva2_vc1.c > index ba6416b..f8b74a2 100644 > --- a/libavcodec/dxva2_vc1.c > +++ b/libavcodec/dxva2_vc1.c > @@ -68,7 +68,7 @@ static void fill_picture_parameters(AVCodecContext *avctx, > pp->bPicStructure |= 0x01; > if (s->picture_structure & PICT_BOTTOM_FIELD) > pp->bPicStructure |= 0x02; > - pp->bSecondField = v->interlace && v->fcm != > 0x03 && !s->first_field; > + pp->bSecondField = v->interlace && v->fcm != > ILACE_FIELD && !s->first_field; The phrases as the spec mentions them are "Interlaced Frame Picture" and "Interlaced Field Picture" (note the ED). The existing code has three different variations. (Here I will have to admit that the mistake is mine.) But since you are documenting, at least trying to be consistent in the newly added lines will be a good idea. > pp->bPicIntra = s->pict_type == AV_PICTURE_TYPE_I; > pp->bPicBackwardPrediction = s->pict_type == AV_PICTURE_TYPE_B; > pp->bBidirectionalAveragingMode = (1 > << 7) | > @@ -100,7 +100,7 @@ static void fill_picture_parameters(AVCodecContext *avctx, > (s->resync_marker << 4) | > (v->rangered << 3) | > (s->max_b_frames ); > - pp->bPicExtrapolation = (!v->interlace || v->fcm == 0x00) ? > 1 : 2; > + pp->bPicExtrapolation = (!v->interlace || v->fcm == > PROGRESSIVE) ? 1 : 2; > pp->bPicDeblocked = ((v->profile != PROFILE_ADVANCED > && v->rangeredfrm) << 5) | > (s->loop_filter > << 1); > pp->bPicDeblockConfined = (v->postprocflag << > 7) | > diff --git a/libavcodec/vc1.c b/libavcodec/vc1.c > index cb228e6..d728f9b 100644 > --- a/libavcodec/vc1.c > +++ b/libavcodec/vc1.c > @@ -836,14 +836,14 @@ int vc1_parse_frame_header_adv(VC1Context *v, > GetBitContext* gb) > if (v->interlace) { > v->fcm = decode012(gb); > if (v->fcm) { > - if (v->fcm == 2) > + if (v->fcm == ILACE_FIELD) > v->field_mode = 1; > if (!v->warn_interlaced++) > av_log(v->s.avctx, AV_LOG_ERROR, > "Interlaced frames/fields support is > incomplete\n"); > } > } else { > - v->fcm = 0; > + v->fcm = PROGRESSIVE; > } > > if (v->field_mode) { > @@ -957,7 +957,7 @@ int vc1_parse_frame_header_adv(VC1Context *v, > GetBitContext* > gb) > switch (v->s.pict_type) { > case AV_PICTURE_TYPE_I: > case AV_PICTURE_TYPE_BI: > - if (v->fcm == 1) { //interlace frame picture > + if (v->fcm == ILACE_FRAME) { //interlace frame picture Now that we have an enum, the comments, imho, are just gratuitous. Also, if we are keeping the comment, it should say "Interlaced Frame Picture" not "interlace frame picture". (More below) > status = bitplane_decoding(v->fieldtx_plane, > &v->fieldtx_is_raw, v); > if (status < 0) > return -1; > @@ -998,7 +998,7 @@ int vc1_parse_frame_header_adv(VC1Context *v, > GetBitContext* > gb) > v->dmvrange = get_unary(gb, 0, 3); > else > v->dmvrange = 0; > - if (v->fcm == 1) { // interlaced frame picture > + if (v->fcm == ILACE_FRAME) { // interlaced frame picture Above comment applies here too. > v->fourmvswitch = get_bits1(gb); > v->intcomp = get_bits1(gb); > if (v->intcomp) { > @@ -1038,7 +1038,7 @@ int vc1_parse_frame_header_adv(VC1Context *v, > GetBitContext* gb) > v->tt_index = 1; > else > v->tt_index = 2; > - if (v->fcm != 1) { > + if (v->fcm != ILACE_FRAME) { > int mvmode; > mvmode = get_unary(gb, 1, 4); > lowquant = (v->pq > 12) ? 0 : 1; > @@ -1073,7 +1073,7 @@ int vc1_parse_frame_header_adv(VC1Context *v, > GetBitContext* gb) > || (v->mv_mode == MV_PMODE_INTENSITY_COMP > && v->mv_mode2 == > MV_PMODE_1MV_HPEL_BILIN)); > } > - if (v->fcm == 0) { // progressive > + if (v->fcm == PROGRESSIVE) { // progressive This just looks stupid. > if ((v->mv_mode == MV_PMODE_INTENSITY_COMP && > v->mv_mode2 == MV_PMODE_MIXED_MV) > || v->mv_mode == MV_PMODE_MIXED_MV) { > @@ -1095,7 +1095,7 @@ int vc1_parse_frame_header_adv(VC1Context *v, > GetBitContext* gb) > /* Hopefully this is correct for P frames */ > v->s.mv_table_index = get_bits(gb, 2); //but using ff_vc1_ > tables > v->cbpcy_vlc = &ff_vc1_cbpcy_p_vlc[get_bits(gb, 2)]; > - } else if (v->fcm == 1) { // frame interlaced > + } else if (v->fcm == ILACE_FRAME) { // frame interlaced See the above note regarding redundant comment and "Interlaced Frame Picture" vs. "frame interlaced picture". "frame interlaced picture" is probably not the correct phrase. > v->qs_last = v->s.quarter_sample; > v->s.quarter_sample = 1; > v->s.mspel = 1; > @@ -1135,7 +1135,7 @@ int vc1_parse_frame_header_adv(VC1Context *v, > GetBitContext* gb) > break; > case AV_PICTURE_TYPE_B: > // TODO: implement interlaced frame B picture decoding > - if (v->fcm == 1) > + if (v->fcm == ILACE_FRAME) > return -1; > if (v->extended_mv) > v->mvrange = get_unary(gb, 0, 3); > diff --git a/libavcodec/vc1.h b/libavcodec/vc1.h > index ac65348..6096077 100644 > --- a/libavcodec/vc1.h > +++ b/libavcodec/vc1.h > @@ -161,6 +161,16 @@ enum COTypes { > }; > //@} > > +/** > + * FCM Frame Coding Mode > + * @note some content might be marked interlaced > + * but have fcm set to 0 as well (e.g. HD-DVD) > + */ The comment is not clear. FCM says if a Picture (Frame, really) is progressive or field-coded (interlaced field picture) or frame- coded (interlaced frame picture). Now obviously streams that have been marked as progressive (syntax element INTERLACE = 0) are not allowed to have interlaced coded pictures. But interlaced stream (INTERLACE = 1) are not bound to such restrictions. They can switch between Progressive-coding / Frame-coding / Field-coding from picture to picture (There is even a name in the spec for such streams, FFAuto :P) > +enum FrameCodingMode { > + PROGRESSIVE = 0, ///< in the bitstream is reported as 00b > + ILACE_FRAME, ///< in the bitstream is reported as 10b > + ILACE_FIELD ///< in the bitstream is reported as 11b > +}; > > /** The VC1 Context > * @todo Change size wherever another size is more efficient > @@ -296,7 +306,7 @@ typedef struct VC1Context{ > > /** Frame decoding info for Advanced profile */ > //@{ > - uint8_t fcm; ///< 0->Progressive, 2->Frame-Interlace, > 3->Field-Interlace > + enum FrameCodingMode fcm; > uint8_t numpanscanwin; > uint8_t tfcntr; > uint8_t rptfrm, tff, rff; > diff --git a/libavcodec/vc1dec.c b/libavcodec/vc1dec.c > index 7a65f31..8f680ea 100644 > --- a/libavcodec/vc1dec.c > +++ b/libavcodec/vc1dec.c > @@ -501,7 +501,8 @@ static void vc1_mc_1mv(VC1Context *v, int dir) > uvmy = uvmy - 2 + 4 * v->cur_field_type; > } > > - if (v->fastuvmc && (v->fcm != 1)) { // fastuvmc shall be > ignored for interlaced frame picture > + // fastuvmc shall be ignored for interlaced frame picture > + if (v->fastuvmc && (v->fcm != ILACE_FRAME)) { > uvmx = uvmx + ((uvmx < 0) ? (uvmx & 1) : -(uvmx & 1)); > uvmy = uvmy + ((uvmy < 0) ? (uvmy & 1) : -(uvmy & 1)); > } > @@ -685,7 +686,7 @@ static void vc1_mc_4mv_luma(VC1Context *v, int n, int dir) > uint8_t *srcY; > int dxy, mx, my, src_x, src_y; > int off; > - int fieldmv = (v->fcm == 1) ? v->blk_mv_type[s->block_index[n]] : > 0; > + int fieldmv = (v->fcm == ILACE_FRAME) ? > v->blk_mv_type[s->block_index[n]] : 0; > int v_edge_pos = s->v_edge_pos >> v->field_mode; > > if (!v->field_mode && !v->s.last_picture.f.data[0]) > @@ -744,7 +745,7 @@ static void vc1_mc_4mv_luma(VC1Context *v, int n, int dir) > v->mv_f[1][s->block_index[k] + v->blocks_off] = f; > } > > - if (v->fcm == 1) { // not sure if needed for other types of picture > + if (v->fcm == ILACE_FRAME) { // not sure if needed for other types of > picture > int qx, qy; > int width = s->avctx->coded_width; > int height = s->avctx->coded_height >> 1; > @@ -761,7 +762,7 @@ static void vc1_mc_4mv_luma(VC1Context *v, int n, int dir) > my -= 8 * (qy - height - 1); > } > > - if ((v->fcm == 1) && fieldmv) > + if ((v->fcm == ILACE_FRAME) && fieldmv) Is that extra pair of brackets necessary? (Again, not implying that it's your mistake.) > off = ((n > 1) ? s->linesize : 0) + (n & 1) * 8; > else > off = s->linesize * 4 * (n & 2) + (n & 1) * 8; > @@ -779,7 +780,7 @@ static void vc1_mc_4mv_luma(VC1Context *v, int n, int dir) > src_y = av_clip(src_y, -16, s->mb_height * 16); > } else { > src_x = av_clip(src_x, -17, s->avctx->coded_width); > - if (v->fcm == 1) { > + if (v->fcm == ILACE_FRAME) { > if (src_y & 1) > src_y = av_clip(src_y, -17, s->avctx->coded_height + 1); > else > @@ -2917,7 +2918,7 @@ static int vc1_decode_i_block_adv(VC1Context *v, > DCTELEM > block[64], int n, > int k; > > if (v->s.ac_pred) { > - if (!use_pred && v->fcm == 1) { > + if (!use_pred && v->fcm == ILACE_FRAME) { > zz_table = v->zzi_8x8; > } else { > if (!dc_pred_dir) // top > @@ -2926,7 +2927,7 @@ static int vc1_decode_i_block_adv(VC1Context *v, > DCTELEM > block[64], int n, > zz_table = v->zz_8x8[3]; > } > } else { > - if (v->fcm != 1) > + if (v->fcm != ILACE_FRAME) > zz_table = v->zz_8x8[1]; > else > zz_table = v->zzi_8x8; > @@ -3136,10 +3137,10 @@ static int vc1_decode_intra_block(VC1Context *v, > DCTELEM > block[64], int n, > i += skip; > if (i > 63) > break; > - if (v->fcm == 0) > + if (v->fcm == PROGRESSIVE) > block[v->zz_8x8[0][i++]] = value; > else { > - if (use_pred && (v->fcm == 1)) { > + if (use_pred && (v->fcm == ILACE_FRAME)) { > if (!dc_pred_dir) // top > block[v->zz_8x8[2][i++]] = value; > else // left > @@ -4739,12 +4740,12 @@ static void vc1_decode_p_blocks(VC1Context *v) > for (; s->mb_x < s->mb_width; s->mb_x++) { > ff_update_block_index(s); > > - if (v->fcm == 2) > + if (v->fcm == ILACE_FIELD) > vc1_decode_p_mb_intfi(v); > - else if (v->fcm == 1) > + else if (v->fcm == ILACE_FRAME) > vc1_decode_p_mb_intfr(v); > else vc1_decode_p_mb(v); > - if (s->mb_y != s->start_mb_y && apply_loop_filter > && v->fcm == 0) > + if (s->mb_y != s->start_mb_y && apply_loop_filter > && v->fcm == PROGRESSIVE) > vc1_apply_p_loop_filter(v); > if (get_bits_count(&s->gb) > v->bits || > get_bits_count(&s->gb) < 0) { > // TODO: may need modification to handle slice coding > @@ -4811,7 +4812,7 @@ static void vc1_decode_b_blocks(VC1Context *v) > for (; s->mb_x < s->mb_width; s->mb_x++) { > ff_update_block_index(s); > > - if (v->fcm == 2) > + if (v->fcm == ILACE_FIELD) > vc1_decode_b_mb_intfi(v); > else > vc1_decode_b_mb(v); > -- > 1.7.8.rc1 > The patch is not wrong per se, it's just that I feel it needs more work. Regards Mashiat Sarker Shakkhar _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
