[FFmpeg-devel] [PATCH] Implement PAFF in H.264
Michael Niedermayer
michaelni
Wed Oct 3 01:19:59 CEST 2007
Hi
On Tue, Oct 02, 2007 at 11:57:15AM -0400, Jeff Downs wrote:
> On Tue, 2 Oct 2007, Andreas ?an wrote:
[...]
> Patch w/out that hunk attached.
>
> Thanks for taking a look.
>
> -Jeff
> --- ../ffmpeg-cosmetics/libavcodec/h264.c 2007-10-01 14:02:06.000000000 -0400
> +++ libavcodec/h264.c 2007-10-01 15:56:22.000000000 -0400
> @@ -175,11 +175,11 @@
> if(for_deblock && (h->slice_num == 1 || h->slice_table[mb_xy] == h->slice_table[mb_xy-s->mb_stride]) && !FRAME_MBAFF)
> return;
>
> //wow what a mess, why didn't they simplify the interlacing&intra stuff, i can't imagine that these complex rules are worth it
>
> - top_xy = mb_xy - s->mb_stride;
> + top_xy = mb_xy - (s->mb_stride << FIELD_PICTURE);
could mb_stride itself be <<=1 ?
or are there many uses which need it like it is even for field pics?
> topleft_xy = top_xy - 1;
> topright_xy= top_xy + 1;
> left_xy[1] = left_xy[0] = mb_xy-1;
> left_block[0]= 0;
> left_block[1]= 1;
> @@ -1701,11 +1701,11 @@
> int extra_height= h->emu_edge_height;
> int emu=0;
> const int full_mx= mx>>2;
> const int full_my= my>>2;
> const int pic_width = 16*s->mb_width;
> - const int pic_height = 16*s->mb_height >> MB_MBAFF;
> + const int pic_height = 16*s->mb_height >> (MB_MBAFF || FIELD_PICTURE);
>
> if(!pic->data[0]) //FIXME this is unacceptable, some senseable error concealment must be done for missing reference frames
> return;
>
> if(mx&7) extra_width -= 3;
> @@ -1725,11 +1725,11 @@
> qpix_op[luma_xy](dest_y + delta, src_y + delta, h->mb_linesize);
> }
>
> if(ENABLE_GRAY && s->flags&CODEC_FLAG_GRAY) return;
>
> - if(MB_MBAFF){
> + if(MB_MBAFF || FIELD_PICTURE){
> // chroma offset when predicting from a field of opposite parity
> my += 2 * ((s->mb_y & 1) - (h->ref_cache[list][scan8[n]] & 1));
> emu |= (my>>3) < 0 || (my>>3) + 8 >= (pic_height>>1);
> }
> src_cb= pic->data[1] + (mx>>3) + (my>>3)*h->mb_uvlinesize;
> @@ -1760,11 +1760,11 @@
>
> dest_y += 2*x_offset + 2*y_offset*h-> mb_linesize;
> dest_cb += x_offset + y_offset*h->mb_uvlinesize;
> dest_cr += x_offset + y_offset*h->mb_uvlinesize;
> x_offset += 8*s->mb_x;
> - y_offset += 8*(s->mb_y >> MB_MBAFF);
> + y_offset += 8*(s->mb_y >> (MB_MBAFF || FIELD_PICTURE));
>
> if(list0){
> Picture *ref= &h->ref_list[0][ h->ref_cache[0][ scan8[n] ] ];
> mc_dir_part(h, ref, n, square, chroma_height, delta, 0,
> dest_y, dest_cb, dest_cr, x_offset, y_offset,
> @@ -1793,11 +1793,11 @@
>
> dest_y += 2*x_offset + 2*y_offset*h-> mb_linesize;
> dest_cb += x_offset + y_offset*h->mb_uvlinesize;
> dest_cr += x_offset + y_offset*h->mb_uvlinesize;
> x_offset += 8*s->mb_x;
> - y_offset += 8*(s->mb_y >> MB_MBAFF);
> + y_offset += 8*(s->mb_y >> (MB_MBAFF || FIELD_PICTURE));
>
> if(list0 && list1){
> /* don't optimize for luma-only case, since B-frames usually
> * use implicit weights => chroma too. */
> uint8_t *tmp_cb = s->obmc_scratchpad;
these can be commited if we have a FIELD_PICTURE=0
> @@ -2247,10 +2247,11 @@
> int i;
>
> if(MPV_frame_start(s, s->avctx) < 0)
> return -1;
> ff_er_frame_start(s);
> + s->current_picture_ptr->key_frame= 0;
>
this line could benefit from a comment explaining how it is/has to be
initalized for field_pics
[...]
> +static int split_field_copy(Picture *dest, Picture *src,
> + int parity, int id_add){
> + int match = (src->reference & parity) != 0;
!!(src->reference & parity);
[...]
> + int same_i, opp_i;
> + int i;
> + int same;
> + int out_i;
> +
> + same_i = 0;
> + opp_i = 0;
> + same = 1;
> + out_i = 0;
declaration and initalization can be merged
> +
> + while (out_i < dest_len) {
> + if (same && same_i < src_len) {
> + i = split_field_copy(dest + out_i, src + same_i, parity, 1);
> + same = !i;
> + same_i++;
> +
> + } else if (opp_i < src_len) {
> + i = split_field_copy(dest + out_i, src + opp_i,
> + PICT_FRAME - parity, 0);
> + same = i;
> + opp_i++;
> +
> + } else {
> + break;
> + }
> + out_i += i;
> + }
for(out_i = 0; out_i < dest_len; out_i += i)
[...]
> + if (!FIELD_PICTURE) {
> + structure_sel = 0;
> + frame_list[0] = h->default_ref_list[0];
> + frame_list[1] = h->default_ref_list[1];
> + } else {
> + structure_sel = PICT_FRAME;
> + frame_list[0] = field_entry_list[0];
> + frame_list[1] = field_entry_list[1];
> + }
if(FIELD_PICTURE){
}else{
}
is a "not" less and IMHO a tiny bit easier to understand
[...]
> + if (FIELD_PICTURE) {
> + frame_num = pred >> 1;
> + if (pred & 1) {
> + pic_structure = s->picture_structure;
> + bot = (s->picture_structure == PICT_BOTTOM_FIELD);
> + } else {
> + pic_structure = PICT_FRAME - s->picture_structure;
> + bot = (s->picture_structure == PICT_TOP_FIELD);
> + }
> + } else {
> + frame_num = pred;
> + pic_structure = PICT_FRAME;
> + bot = 0;
> + }
> +
> for(i= h->short_ref_count-1; i>=0; i--){
> ref = h->short_ref[i];
> - assert(ref->reference == 3);
> + assert(ref->reference);
> assert(!ref->long_ref);
> - if(ref->data[0] != NULL && ref->frame_num == pred && ref->long_ref == 0) // ignore non existing pictures by testing data[0] pointer
> + if(ref->data[0] != NULL &&
> + ref->frame_num == frame_num &&
> + (ref->reference & pic_structure) &&
> + ref->long_ref == 0) // ignore non existing pictures by testing data[0] pointer
> break;
> }
> if(i>=0)
> - ref->pic_id= ref->frame_num;
> + ref->pic_id= pred;
> }else{
> + int long_idx;
> pic_id= get_ue_golomb(&s->gb); //long_term_pic_idx
> - if(pic_id>31){
> +
> + if (FIELD_PICTURE) {
> + long_idx = pic_id >> 1;
> + if (pic_id & 1) {
> + pic_structure = s->picture_structure;
> + bot = (s->picture_structure == PICT_BOTTOM_FIELD);
> + } else {
> + pic_structure = PICT_FRAME - s->picture_structure;
> + bot = (s->picture_structure == PICT_TOP_FIELD);
> + }
> + } else {
> + long_idx = pic_id;
> + pic_structure = PICT_FRAME;
> + bot = 0;
> + }
this looks duplicated
[...]
> -static inline void unreference_pic(H264Context *h, Picture *pic){
> +/**
> + * Mark a picture as no longer needed for reference. The refmask
> + * argument allows unreferencing of individual fields or the whole frame.
> + * If the picture becomes entirely unreferenced, but is being held for
> + * display purposes, it is marked as such.
> + * @param refmask mask of fields to unreference; the mask is bitwise
> + * anded with the reference marking of pic
> + * @return non-zero if pic becomes entirely unreferenced (except possibly
> + * for display purposes) zero if one of the fields remains in
> + * reference
> + */
> +static inline int unreference_pic(H264Context *h, Picture *pic, int refmask){
> int i;
> - pic->reference=0;
> + if (pic->reference &= refmask) {
> + return 0;
> + } else {
> if(pic == h->delayed_output_pic)
> pic->reference=DELAYED_PIC_REF;
> else{
> for(i = 0; h->delayed_pic[i]; i++)
> if(pic == h->delayed_pic[i]){
> pic->reference=DELAYED_PIC_REF;
> break;
> }
> }
> + return 1;
> + }
> }
this looks ok, if it where in a seperate patch with the related
unreference_pic() call changes that could be commited
[...]
> /**
> - *
> - * @return the removed picture or NULL if an error occurs
> + * Find a Picture in the short term reference list by frame number.
> + * @param frame_num frame number to search for
> + * @param idx the index into h->short_ref where returned picture is found
> + * undefined if no picture found.
> + * @return pointer to the found picture, or NULL if no pic with the provided
> + * frame number is found
> */
> -static Picture * remove_short(H264Context *h, int frame_num){
> +static Picture * find_short(H264Context *h, int frame_num, int *idx){
> MpegEncContext * const s = &h->s;
> int i;
>
> - if(s->avctx->debug&FF_DEBUG_MMCO)
> - av_log(h->s.avctx, AV_LOG_DEBUG, "remove short %d count %d\n", frame_num, h->short_ref_count);
> -
> for(i=0; i<h->short_ref_count; i++){
> Picture *pic= h->short_ref[i];
> if(s->avctx->debug&FF_DEBUG_MMCO)
> av_log(h->s.avctx, AV_LOG_DEBUG, "%d %d %p\n", i, pic->frame_num, pic);
> - if(pic->frame_num == frame_num){
> - h->short_ref[i]= NULL;
> - if (--h->short_ref_count)
> - memmove(&h->short_ref[i], &h->short_ref[i+1], (h->short_ref_count - i)*sizeof(Picture*));
> + if(pic->frame_num == frame_num) {
> + *idx = i;
> return pic;
> }
> }
> return NULL;
> }
this split out could also be in its own patch
[...]
> - unsigned int long_index= get_ue_golomb(gb);
> - if(/*h->mmco[i].long_arg >= h->long_ref_count || h->long_ref[ h->mmco[i].long_arg ] == NULL*/ long_index >= 16){
> + unsigned int long_arg= get_ue_golomb(gb);
> + if(long_arg >= 32 || (long_arg >= 16 && !(opcode == MMCO_LONG2UNUSED && FIELD_PICTURE))){
> av_log(h->s.avctx, AV_LOG_ERROR, "illegal long ref in memory management control operation %d\n", opcode);
> return -1;
> }
> - h->mmco[i].long_arg= long_index;
> + h->mmco[i].long_arg= long_arg;
this renaming should be in a seperate patch
[...]
> }
> +
> if(h != h0)
cosmetic
[...]
> - //FIXME do something with unavailable reference frames
[...]
> + //FIXME do something with unavailable reference frames
cosmetic
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- 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/20071003/fda67a90/attachment.pgp>
More information about the ffmpeg-devel
mailing list