[FFmpeg-devel] [GSoC 2009] [Patch] Optimal Huffman tables for (M)JPEG encoding
Michael Niedermayer
michaelni
Mon Apr 6 16:36:22 CEST 2009
On Sun, Apr 05, 2009 at 11:34:51PM -0400, Indrani Kundu Saha wrote:
> I am attaching the patch for the GSoC 2009 qual task. It implements
> the complete functional path but the patch has some bugs. I observed
> the following behaviour that I am trying to debug:
>
> 1. The freq of AC/DC symbols is unusually large. The code is picking
> some symbols which are not in the original encoded frame.
> 2. It seg faults for grey images and most color images esp those with
> large number of AC values.
>
>
> While I am trying to debug these and more, suggestions and comments
> are very much welcome.
trailing whitespace
gsoc2009_huffman.patch:71:+
gsoc2009_huffman.patch:87:+
gsoc2009_huffman.patch:137:+
gsoc2009_huffman.patch:246:+
gsoc2009_huffman.patch:283:+{
gsoc2009_huffman.patch:286:+
gsoc2009_huffman.patch:290:+
gsoc2009_huffman.patch:310:+
gsoc2009_huffman.patch:314:+static void ff_mjpeg_build_code_words(MJpegHuffTable *tbl_ptr,
gsoc2009_huffman.patch:315:+ MJpegHuffNode *node,
gsoc2009_huffman.patch:319:+
gsoc2009_huffman.patch:352:+ ptr1 = &huff.buf[0] + put_bits_count(&huff)/8;
gsoc2009_huffman.patch:353:+
gsoc2009_huffman.patch:363:+ }
gsoc2009_huffman.patch:368:+
gsoc2009_huffman.patch:377:+ }
gsoc2009_huffman.patch:382:+
gsoc2009_huffman.patch:383:+
gsoc2009_huffman.patch:392:+ }
gsoc2009_huffman.patch:406:+ }
gsoc2009_huffman.patch:424:+
gsoc2009_huffman.patch:446:+
gsoc2009_huffman.patch:448:+ ptr1 = &huff.buf[0] + put_bits_count(&huff)/8;
gsoc2009_huffman.patch:456:+
gsoc2009_huffman.patch:470:+ }
gsoc2009_huffman.patch:471:+
tabs
gsoc2009_huffman.patch:104:+ table_ptr->bits[i] = bits_table[i];
gsoc2009_huffman.patch:111:+ table_ptr->code[i] = value_table[i];
gsoc2009_huffman.patch:166:+ ac_ptr = &opt_huff_ac_luminance;
gsoc2009_huffman.patch:168:+ /* Store the start and end of DC Luminance */
gsoc2009_huffman.patch:169:+ opt_huff_dc_luminance.start_of_buffer = size;
gsoc2009_huffman.patch:170:+ opt_huff_dc_luminance.end_of_buffer = put_bits_count(&s->pb)>>3;
gsoc2009_huffman.patch:171:+ ff_mjpeg_extract_buffer((&s->pb)->buf,size,put_bits_count(&s->pb)>>3,&opt_huff_dc_luminance);
gsoc2009_huffman.patch:176:+ ac_ptr = &opt_huff_ac_chrominance;
gsoc2009_huffman.patch:178:+ /* Store the start and end of DC Chrominance */
gsoc2009_huffman.patch:179:+ opt_huff_dc_chrominance.start_of_buffer = size;
gsoc2009_huffman.patch:180:+ opt_huff_dc_chrominance.end_of_buffer = put_bits_count(&s->pb)>>3;
gsoc2009_huffman.patch:181:+ ff_mjpeg_extract_buffer((&s->pb)->buf,size,put_bits_count(&s->pb)>>3,&opt_huff_dc_chrominance);
gsoc2009_huffman.patch:196:+ ff_mjpeg_extract_buffer((&s->pb)->buf,size,put_bits_count(&s->pb)>>3,ac_ptr);
gsoc2009_huffman.patch:204:+ ff_mjpeg_extract_buffer((&s->pb)->buf,size,put_bits_count(&s->pb)>>3,ac_ptr);
gsoc2009_huffman.patch:207:+ ff_mjpeg_extract_buffer((&s->pb)->buf,size,put_bits_count(&s->pb)>>3,ac_ptr);
gsoc2009_huffman.patch:260:+ ff_mjpeg_insert_val(buffer[i],ptr);
gsoc2009_huffman.patch:261:+ }
gsoc2009_huffman.patch:270:+ ptr->symtable[i].sym = val;
gsoc2009_huffman.patch:271:+ ptr->symtable[i].count = 1;
gsoc2009_huffman.patch:272:+ ptr->symbol_count++;
gsoc2009_huffman.patch:273:+ return;
gsoc2009_huffman.patch:276:+ return;
gsoc2009_huffman.patch:277:+ }
gsoc2009_huffman.patch:290:+
gsoc2009_huffman.patch:292:+ memcpy (&tbl_ptr->tree[tree_index+1],&tbl_ptr->symtable[i-1],sizeof(MJpegHuffNode));
gsoc2009_huffman.patch:293:+ i = i-1;
gsoc2009_huffman.patch:295:+ tbl_ptr->symtable[i].node_type = FF_MJPEG_DUMMY_HUFF_NODE;
gsoc2009_huffman.patch:296:+ tbl_ptr->symtable[i].count += tbl_ptr->symtable[i+1].count;
gsoc2009_huffman.patch:298:+ tbl_ptr->symtable[i].lchild = tree_index;
gsoc2009_huffman.patch:299:+ tbl_ptr->symtable[i].rchild = tree_index+1;
gsoc2009_huffman.patch:301:+ tree_index += 2;
gsoc2009_huffman.patch:303:+ total_symbols--;
gsoc2009_huffman.patch:305:+ /* This can be optimized. We do not need to sort the entire array. A few sort
gsoc2009_huffman.patch:306:+ * operations hould be alright.. TODO
gsoc2009_huffman.patch:307:+ */
gsoc2009_huffman.patch:308:+ qsort(&tbl_ptr->symtable[0], total_symbols, sizeof(MJpegHuffNode), ff_mjpeg_huff_cmp);
gsoc2009_huffman.patch:315:+ MJpegHuffNode *node,
gsoc2009_huffman.patch:316:+ uint32_t code, uint32_t depth)
gsoc2009_huffman.patch:322:+ ff_mjpeg_build_code_words(tbl_ptr, &tbl_ptr->tree[node->lchild],code<<1,depth);
gsoc2009_huffman.patch:323:+ ff_mjpeg_build_code_words(tbl_ptr, &tbl_ptr->tree[node->rchild],code|=1,depth);
gsoc2009_huffman.patch:326:+ tbl_ptr->code_table[code_index].code = code;
gsoc2009_huffman.patch:327:+ tbl_ptr->code_table[code_index].code_length = depth;
gsoc2009_huffman.patch:328:+ code_index++;
gsoc2009_huffman.patch:356:+ ptr!=s->pb.buf + opt_huff_dc_luminance.end_of_buffer; ptr++){
gsoc2009_huffman.patch:358:+ for (j=0; j<256; j++){
gsoc2009_huffman.patch:359:+ if (opt_huff_dc_luminance.code_table[j].sym == *ptr){
gsoc2009_huffman.patch:360:+ *ptr1 = opt_huff_dc_luminance.code_table[j].code;
gsoc2009_huffman.patch:361:+ ptr1++;
gsoc2009_huffman.patch:362:+ }
gsoc2009_huffman.patch:363:+ }
gsoc2009_huffman.patch:364:+ }
gsoc2009_huffman.patch:372:+ for (j=0; j<256; j++){
gsoc2009_huffman.patch:373:+ if (opt_huff_dc_chrominance.code_table[j].sym == *ptr){
gsoc2009_huffman.patch:374:+ *ptr1 = opt_huff_dc_chrominance.code_table[j].code;
gsoc2009_huffman.patch:375:+ ptr1++;
gsoc2009_huffman.patch:376:+ }
gsoc2009_huffman.patch:377:+ }
gsoc2009_huffman.patch:378:+ }
gsoc2009_huffman.patch:387:+ for (j=0; j<256; j++){
gsoc2009_huffman.patch:388:+ if (opt_huff_ac_luminance.code_table[j].sym == *ptr){
gsoc2009_huffman.patch:389:+ *ptr1 = opt_huff_ac_luminance.code_table[j].code;
gsoc2009_huffman.patch:390:+ ptr1++;
gsoc2009_huffman.patch:391:+ }
gsoc2009_huffman.patch:392:+ }
gsoc2009_huffman.patch:393:+ }
gsoc2009_huffman.patch:401:+ for (j=0; j<256; j++){
gsoc2009_huffman.patch:402:+ if (opt_huff_ac_chrominance.code_table[j].sym == *ptr){
gsoc2009_huffman.patch:403:+ *ptr1 = opt_huff_ac_chrominance.code_table[j].code;
gsoc2009_huffman.patch:404:+ ptr1++;
gsoc2009_huffman.patch:405:+ }
gsoc2009_huffman.patch:406:+ }
gsoc2009_huffman.patch:407:+ }
gsoc2009_huffman.patch:443:+ &opt_huff_ac_chrominance.DHTTable.code);
gsoc2009_huffman.patch:460:+ for (j=0; j<256; j++){
gsoc2009_huffman.patch:461:+ if (tbl_ptr->DHTTable.code[j] == tbl_ptr->code_table[i].code){
gsoc2009_huffman.patch:462:+ code_exists = 1;
gsoc2009_huffman.patch:464:+ }
gsoc2009_huffman.patch:465:+ }
gsoc2009_huffman.patch:466:+ if (code_exists == 0){
gsoc2009_huffman.patch:467:+ tbl_ptr->DHTTable.bits[tbl_ptr->code_table[i].code_length]++;
gsoc2009_huffman.patch:468:+ tbl_ptr->DHTTable.code[tbl_ptr->DHTTable.index] = tbl_ptr->code_table[i].code;
gsoc2009_huffman.patch:469:+ tbl_ptr->DHTTable.index++;
gsoc2009_huffman.patch:470:+ }
gsoc2009_huffman.patch:471:+
gsoc2009_huffman.patch:472:+ }
double ;
gsoc2009_huffman.patch:187:+ 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:338:+static void ff_mjpeg_init_MJpegHuffTable(MJpegHuffTable *ptr)
x==0 / x!=0 can be simplified to !x / x
gsoc2009_huffman.patch:89:+ if (table_class == 0 && table_id == 0)
gsoc2009_huffman.patch:91:+ else if (table_class == 0 && table_id == 1)
gsoc2009_huffman.patch:93:+ else if (table_class == 1 && table_id == 0)
gsoc2009_huffman.patch:466:+ if (code_exists == 0){
useless 0 init
gsoc2009_huffman.patch:318:+ static int code_index = 0;
divide by 2^x could use >> maybe
gsoc2009_huffman.patch:352:+ ptr1 = &huff.buf[0] + put_bits_count(&huff)/8;
gsoc2009_huffman.patch:411:+ memcpy (s->pb.buf,huff.buf, put_bits_count(&huff)/8);
gsoc2009_huffman.patch:448:+ 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_build_code_words(MJpegHuffTable *tbl_ptr, MJpegHuffNode *node, uint32_t code, uint32_t depth);
gsoc2009_huffman.patch:51:+static void ff_mjpeg_init_MJpegHuffTable(MJpegHuffTable *ptr);
gsoc2009_huffman.patch:52:+static void ff_mjpeg_build_huff_tree(MJpegHuffTable *tbl_ptr);
gsoc2009_huffman.patch:53:+static void ff_mjpeg_insert_val(int val, MJpegHuffTable *ptr);
gsoc2009_huffman.patch:54:+static int ff_mjpeg_huff_cmp(const void *va, const void *vb);
gsoc2009_huffman.patch:55:+static void ff_mjpeg_update_DHT(MJpegHuffTable *tbl_ptr);
gsoc2009_huffman.patch:56:+static void ff_mjpeg_write_DHT(MpegEncContext *s);
gsoc2009_huffman.patch:57:+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:91:+ else if (table_class == 0 && table_id == 1)
gsoc2009_huffman.patch:93:+ else if (table_class == 1 && table_id == 0)
gsoc2009_huffman.patch:95:+ else if (table_class == 1 && table_id == 1)
Missing changelog entry (ignore if minor change)
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
No human being will ever know the Truth, for even if they happen to say it
by chance, they would not even known they had done so. -- Xenophanes
-------------- 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/20090406/87d77c89/attachment.pgp>
More information about the ffmpeg-devel
mailing list