[FFmpeg-devel] [GSoC 2009] [PATCH Optimal Huffman tables for (M)JPEG encoding

[Michael Niedermayer]

On Mon, Apr 06, 2009 at 08:24:51PM -0400, Indrani Kundu Saha wrote:
> This is a new patch file generated without tabs and trailing spaces..

nope, besides i assume it still doesnt work and just segfaults right?

trailing whitespace
gsoc2009_huffman.patch:61:+static void ff_mjpeg_extract_buffer(uint8_t *buffer, 
gsoc2009_huffman.patch:62:+                                    int start, 
gsoc2009_huffman.patch:63:+                                    int end, 
gsoc2009_huffman.patch:348:+    ptr1 = &huff.buf[0] + put_bits_count(&huff)/8; 
gsoc2009_huffman.patch:440:+    ptr1 = &huff.buf[0] + put_bits_count(&huff)/8; 

double ;
gsoc2009_huffman.patch:188:+    ac_ptr->start_of_buffer = put_bits_count (&s->pb)>>3;;

These functions may need av_cold, please review the whole patch for similar functions needing av_cold
gsoc2009_huffman.patch:334:+static void ff_mjpeg_init_MJpegHuffTable(MJpegHuffTable *ptr)

x==0 / x!=0 can be simplified to !x / x
gsoc2009_huffman.patch:94:+    if (table_class == 0 && table_id == 0)
gsoc2009_huffman.patch:96:+    else if (table_class == 0 && table_id == 1)
gsoc2009_huffman.patch:98:+    else if (table_class == 1 && table_id == 0)
gsoc2009_huffman.patch:458:+            if (code_exists == 0){

useless 0 init
gsoc2009_huffman.patch:314:+  static int code_index = 0;

divide by 2^x could use >> maybe
gsoc2009_huffman.patch:348:+    ptr1 = &huff.buf[0] + put_bits_count(&huff)/8; 
gsoc2009_huffman.patch:408:+    memcpy (s->pb.buf,huff.buf,  put_bits_count(&huff)/8);
gsoc2009_huffman.patch:440:+    ptr1 = &huff.buf[0] + put_bits_count(&huff)/8; 

static prototype, maybe you should reorder your functions
gsoc2009_huffman.patch:50:+static void ff_mjpeg_init_MJpegHuffTable(MJpegHuffTable *ptr);
gsoc2009_huffman.patch:51:+static void ff_mjpeg_build_huff_tree(MJpegHuffTable *tbl_ptr);
gsoc2009_huffman.patch:56:+static void ff_mjpeg_insert_val(int val, MJpegHuffTable *ptr);
gsoc2009_huffman.patch:57:+static int ff_mjpeg_huff_cmp(const void *va, const void *vb);
gsoc2009_huffman.patch:58:+static void ff_mjpeg_update_DHT(MJpegHuffTable *tbl_ptr);
gsoc2009_huffman.patch:59:+static void ff_mjpeg_write_DHT(MpegEncContext *s);
gsoc2009_huffman.patch:60:+static void ff_mjpeg_write_reencode(MpegEncContext *s);

Non static with no ff_/av_ prefix
gsoc2009_huffman.patch:45:+int DHT_loc;
gsoc2009_huffman.patch:46:+int buffer_end_of_DHT;
gsoc2009_huffman.patch:48:+PutBitContext huff;

missing } prior to else
gsoc2009_huffman.patch:96:+    else if (table_class == 0 && table_id == 1)
gsoc2009_huffman.patch:98:+    else if (table_class == 1 && table_id == 0)
gsoc2009_huffman.patch:100:+    else if (table_class == 1 && table_id == 1)

Missing changelog entry (ignore if minor change)

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

Avoid a single point of failure, be that a person or equipment.
-------------- 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/20090407/ab88402a/attachment.pgp>