[FFmpeg-devel] [GSOC][PATCH 1/4] factoring obmc out of snow

Michael Niedermayer michael at niedermayer.cc
Thu Aug 18 05:19:07 EEST 2016


On Wed, Aug 17, 2016 at 02:07:20PM +0300, Станислав Долганов wrote:
> Hello,
> 
> I'm sending the patch set with implementation of GSoC project -- FFV1 P
> frame support. The current FFV1 uses the same OBMC code as the Snow codec.
> Also new median_me_mp function has appeared.
> 
> I'm attaching speed&compression report to every patch to proof effectivity
> of each implemented part.
> 
> I'll appreciate feedback
> 
> Best regards,
> Stanislav

>  b/libavcodec/Makefile      |    8 
>  b/libavcodec/obmc.c        |   59 +
>  b/libavcodec/obmc.h        |   30 
>  b/libavcodec/obme.c        | 1133 ++++++++++++++++++++++++++++++++++++
>  b/libavcodec/obme.h        |   33 +
>  b/libavcodec/obmemc.c      |  650 ++++++++++++++++++++
>  b/libavcodec/obmemc.h      |  522 ++++++++++++++++
>  b/libavcodec/obmemc_data.h |  132 ++++
>  b/libavcodec/snow.c        |  571 ------------------
>  b/libavcodec/snow.h        |  341 ----------
>  b/libavcodec/snowdec.c     |  217 +-----
>  b/libavcodec/snowenc.c     | 1411 +++++++--------------------------------------
>  libavcodec/snowdata.h      |  132 ----
>  13 files changed, 2890 insertions(+), 2349 deletions(-)
> 302bd7a57b192a32abc855abc11a86b5b347ceea  0001-factoring-obmc-out-of-snow.patch
> From 6e16cf2f222a3989db71742511a6cc3250a41980 Mon Sep 17 00:00:00 2001
> From: Stanislav Dolganov <dolganov at qst.hk>
> Date: Tue, 16 Aug 2016 15:14:32 +0300
> Subject: [PATCH 1/4] factoring obmc out of snow

> diff --git a/libavcodec/snow.c b/libavcodec/obmemc.c
> similarity index 68%
> copy from libavcodec/snow.c
> copy to libavcodec/obmemc.c
> index a3e6afc..653b8f3 100644
> --- a/libavcodec/snow.c
> +++ b/libavcodec/obmemc.c
>
> @@ -324,7 +215,25 @@ static void mc_block(Plane *p, uint8_t *dst, const uint8_t *src, int stride, int
>      }
>  }
>
> -void ff_snow_pred_block(SnowContext *s, uint8_t *dst, uint8_t *tmp, ptrdiff_t stride, int sx, int sy, int b_w, int b_h, const BlockNode *block, int plane_index, int w, int h){
> +#define mca(dx,dy,b_w)\
> +static void mc_block_hpel ## dx ## dy ## b_w(uint8_t *dst, const uint8_t *src, ptrdiff_t stride, int h){\
> +    av_assert2(h==b_w);\
> +    mc_block(NULL, dst, src-(HTAPS_MAX/2-1)-(HTAPS_MAX/2-1)*stride, stride, b_w, b_w, dx, dy);\
> +}
> +
> +mca( 0, 0,16)
> +mca( 8, 0,16)
> +mca( 0, 8,16)
> +mca( 8, 8,16)
> +mca( 0, 0,8)
> +mca( 8, 0,8)
> +mca( 0, 8,8)
> +mca( 8, 8,8)
> +
> +void ff_obmc_pred_block(OBMCContext *s, uint8_t *dst, uint8_t *tmp, ptrdiff_t stride,
> +                     int sx, int sy, int b_w, int b_h, const BlockNode *block,
> +                     int plane_index, int w, int h)
> +{
>      if(block->type & BLOCK_INTRA){
>          int x, y;
>          const unsigned color  = block->color[plane_index];
> @@ -364,7 +273,7 @@ void ff_snow_pred_block(SnowContext *s, uint8_t *dst, uint8_t *tmp, ptrdiff_t st
>              }
>          }
>      }else{
> -        uint8_t *src= s->last_picture[block->ref]->data[plane_index];
> +        uint8_t *src= s->last_pictures[block->ref]->data[plane_index];
>          const int scale= plane_index ?  (2*s->mv_scale)>>s->chroma_h_shift : 2*s->mv_scale;
>          int mx= block->mx*scale;
>          int my= block->my*scale;
> @@ -412,180 +321,61 @@ void ff_snow_pred_block(SnowContext *s, uint8_t *dst, uint8_t *tmp, ptrdiff_t st
>      }
>  }
>
> -#define mca(dx,dy,b_w)\
> -static void mc_block_hpel ## dx ## dy ## b_w(uint8_t *dst, const uint8_t *src, ptrdiff_t stride, int h){\
> -    av_assert2(h==b_w);\
> -    mc_block(NULL, dst, src-(HTAPS_MAX/2-1)-(HTAPS_MAX/2-1)*stride, stride, b_w, b_w, dx, dy);\
> -}
> -
> -mca( 0, 0,16)
> -mca( 8, 0,16)
> -mca( 0, 8,16)
> -mca( 8, 8,16)
> -mca( 0, 0,8)
> -mca( 8, 0,8)
> -mca( 0, 8,8)
> -mca( 8, 8,8)

Please dont reorder functions when factorizing code, "git diff" cant
handle that at all, making comparing the old vs new hard

the same applied to other parts too (like mcf)


> -static int halfpel_interpol(SnowContext *s, uint8_t *halfpel[4][4], AVFrame *frame){
> +static int halfpel_interpol(OBMCContext *s, uint8_t *halfpel[4][4], AVFrame *frame)
...
>  -int ff_snow_frame_start(SnowContext *s){
>  +int ff_obmc_frame_start(OBMCContext *f)

please dont rename variables in a patch that factors code (like s->f)
it makes the changes hard to review. And makes newly appearing bugs
hard to debug as functional changes are hard to spot


>      DWTELEM *spatial_dwt_buffer;
>      DWTELEM *temp_dwt_buffer;
> -    IDWTELEM *spatial_idwt_buffer;
> +    //IDWTELEM *spatial_idwt_buffer;
>      IDWTELEM *temp_idwt_buffer;
>      int *run_buffer;
>      int colorspace_type;

should be removed entirely not just commented out


> -static inline void set_blocks(SnowContext *s, int level, int x, int y, int l, int cb, int cr, int mx, int my, int ref, int type){
> +/*static inline void snow_set_blocks(SnowContext *s, int level, int x, int y, int l, int cb, int cr, int mx, int my, int ref, int type){

this too looks like a stray forgotten commented piece of code


> @@ -266,7 +198,7 @@ static int encode_q_branch(SnowContext *s, int level, int x, int y){
>     int s_context= 2*left->level + 2*top->level + tl->level + tr->level;
>     int ref, best_ref, ref_score, ref_mx, ref_my;
>
>-    av_assert0(sizeof(s->block_state) >= 256);
>+    //av_assert0(sizeof(s->block_state) >= 256);
>     if(s->keyframe){
>         set_blocks(s, level, x, y, pl, pcb, pcr, 0, 0, 0, BLOCK_INTRA);
>         return 0;

why ?



>-static void encode_blocks(SnowContext *s, int search){
>+static void encode_blocks(OBMCContext *s, int search){
>     int x, y;
>     int w= s->b_width;
>     int h= s->b_height;
>+    //RangeCoder *const c = s->c;
>+    ObmcCoderContext *const c = &s->obmc_coder;
>
>     if(s->motion_est == FF_ME_ITER && !s->keyframe && search)
>         iterative_me(s);
>
>     for(y=0; y<h; y++){
>-        if(s->c.bytestream_end - s->c.bytestream < w*MB_SIZE*MB_SIZE*3){ //FIXME nicer limit
>+        //if(c->bytestream_end - c->bytestream < w*MB_SIZE*MB_SIZE*3){ //FIXME nicer limit
>+        if(c->available_bytes(c) < w*MB_SIZE*MB_SIZE*3){ //FIXME nicer limit

more stray outcommented code

but overall, this is nice work! It needs a bit more cleanup though

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

Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160818/051a62fc/attachment.sig>


More information about the ffmpeg-devel mailing list