[FFmpeg-devel] [PATCH] RoQ video encoder (take 4)

[Michael Niedermayer]

Hi

On Sun, Jun 24, 2007 at 06:52:51PM +0200, Vitor wrote:
> Hi
> 
> Michael Niedermayer wrote:
> [...]
> 
> >>>>+/**
> >>>>+ * Get macroblocks from parts of the image
> >>>>+ */
> >>>>+static void get_frame_mb(AVFrame *frame, int x, int y, uint8_t mb[], 
> >>>>int dim)
> >>>>+{
> >>>>+    int i, j, cp;
> >>>>+    int stride = frame->linesize[0];
> >>>>+
> >>>>+    for (i=0; i<dim; i++)
> >>>>+        for (j=0; j<dim; j++)
> >>>>+            for (cp=0; cp<3; cp++)
> >>>>+                mb[i*3*dim+j*3+cp] = frame->data[cp][(y+i)*stride+x+j];
> >>>>+}
> >>>>   
> >>>why is the planar data converted to packed?
> >>> 
> >>It simplified some code...
> >
> >which code?
> >also a 3 byte compare where 2 are weighted by CHROMA_BIAS is very SIMD
> >unfriendly (=slow)
> 
> Agreed. Converted to planar.
> 
> [...]
> 
> >>+/**
> >>+ * Returns distortion between two macroblocks
> >>+ */
> >>+static inline int squared_diff_macroblock(uint8_t a[], uint8_t b[], int 
> >>size)
> >>+{
> >>+    int sdiff=0;
> >>+    int n, cp, bias;
> >>+
> >>+    for(cp=0;cp<3;cp++) {
> >>+        bias = (cp ? CHROMA_BIAS : 1);
> >>+        for(n=0;n<size*size;n++)
> >>+            sdiff += (bias*square(b[cp+3*n] - a[cp+3*n]) + 2)/4;
> >>+    }
> >>+
> >>+    return sdiff;
> >>+}
> >
> >duplicate of block_sse()
> 
> I didn't find a nice way to avoid duplication (one gets uint_8 ** as a 
> parameter while the other a uint8_t []), so at least I tried to reuse 
> code...
> 
> >
> >
> >>+
> >>+typedef struct
> >>+{
> >>+    int eval_dist[4];
> >>+    int best_bit_use;
> >>+    int best_coding;
> >>+
> >>+    int subCels[4];
> >>+
> >>+    int motionX, motionY;
> >>+    int cbEntry;
> >>+} subcel_evaluation_t;
> >>+
> >>+typedef struct
> >>+{
> >>+    int eval_dist[4];
> >>+    int best_coding;
> >>+
> >>+    subcel_evaluation_t subCels[4];
> >>+
> >>+    int motionX, motionY;
> >>+    int cbEntry;
> >>+
> >>+    int sourceX, sourceY;
> >>+} cel_evaluation_t;
> >
> >*X/*Y could be int[2] or motion_vect
> >
> >also iam tempted to suggest to merge these 2 structs, they are pretty much 
> >the
> >same, but maybe it cant be done cleanly?
> 
> I didn't found a great way of doing it...
> 
> [...]
> 
> >>+/**
> >>+ * Gets distortion for all options available to a subcel
> >>+ */
> >>+static void gather_data_for_subcel(subcel_evaluation_t *subcel, int x,
> >>+                                   int y, RoqContext *enc, 
> >>roq_tempdata_t *tempData)
> >>+{
> >>+    uint8_t mb4[4*4*3];
> >>+    uint8_t mb2[2*2*3];
> >>+    int cluster_index;
> >>+    int i, best_dist;
> >>+
> >>+    static const int bitsUsed[4] = {2, 10, 10, 34};
> >>+
> >>+    if (enc->framesSinceKeyframe >= 1) {
> >>+        subcel->motionX = enc->this_motion4[y*enc->width/16 + x/4].d[0];
> >>+        subcel->motionY = enc->this_motion4[y*enc->width/16 + x/4].d[1];
> >>+
> >>+        subcel->eval_dist[RoQ_ID_FCC] =
> >>+            eval_motion_dist(enc, x, y,
> >>+                             enc->this_motion4[y*enc->width/16 + 
> >>x/4].d[0],
> >>+                             enc->this_motion4[y*enc->width/16 + 
> >>x/4].d[1], 4);
> >>+    } else
> >>+        subcel->eval_dist[RoQ_ID_FCC] = INT_MAX;
> >
> >maybe setting all eval_dist to INT_MAX at some central place would look
> >simpler then these else cases
> 
> When I read you comment it seemed a good idea, but looking at the code I 
> realized it won't simplify it.
> 
> I've changed as suggested the rest. I've added also a commented debug 
> code. If it is a problem, I can remove it.

patch looks ok

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Democracy is the form of government in which you can choose your dictator
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070625/f32153cf/attachment.pgp>