Hi.
As I understand there is no need for c41.h and c41window.h. I looked at
c41.c
I numbered the lines and left only parts I commented.
<snip>
53
54 int active, compute_magic;
55 float min_r, min_g, min_b, magic4, magic5, magic6;
Move them out from Config as these are always computed values. Config must
cotain only
user changeable parameters
56 float fix_min_r, fix_min_g, fix_min_b, fix_magic4,
fix_magic5, fix_magic6;
Declare every variable on a separate line
57 };
One tab more than needed. Class declaration template
class MyClass {
public:
<tab>MyClass();
private:
<tab>int a_var;
};
169 int C41Config::equivalent(C41Config &src)
170 {
171 return false;
172 }
Why empty function? With the help of this decision is made wether the gui
need to be updated
173 void C41Config::interpolate(C41Config &prev,
174 C41Config &next,
175 long prev_frame,
176 long next_frame,
177 long current_frame)
178 {
179 active = prev.active;
193 fix_magic5 = prev.fix_magic5;
194 fix_magic6 = prev.fix_magic6;
195
196 }
No need for empty line before closing brace
I think it is reasonable to interpolate the values - user may want changing
parameters
197
198 // C41Enable
199 C41Enable::C41Enable(C41Effect *plugin, int *output, int x,
int y, char *text)
200 : BC_CheckBox(x, y, *output, text)
Indentation of a constructor:
MyClass::MyClass()
<space>:<space>ParentConstructor()
201 {
202 this->plugin = plugin;
203 this->output = output;
204 }
One empty line between methods
205 int C41Enable::handle_event()
206 {
246 void C41Window::create_objects()
247 {
260
261 add_subwindow(new BC_Title(x, y, _("Min R:")));
262 add_subwindow(min_r = new C41TextBox(plugin,
&plugin->config.min_r, x+60, y));
263 y += 30;
Use BC_Title instead of C41TextBox, to reflect the fact that min_r,
min_g... are unchangeable
345
346 NEW_PICON_MACRO(C41Effect)
347 SHOW_GUI_MACRO(C41Effect, C41Thread)
348 RAISE_WINDOW_MACRO(C41Effect)
349 SET_STRING_MACRO(C41Effect)
No tab here
350 LOAD_CONFIGURATION_MACRO(C41Effect, C41Config)
351
362
363 void C41Effect::update_gui()
364 {
365 // We don't use update_gui, but rather render_gui.
366 // However, the method is still needed to instantiate the
plugin
350 LOAD_CONFIGURATION_MACRO(C41Effect, C41Config)
351
362
363 void C41Effect::update_gui()
364 {
365 // We don't use update_gui, but rather render_gui.
366 // However, the method is still needed to instantiate the
plugin
Here should be:
if(thread && load_configuration)
{
thread->window->lock_window("C41Effect::update_gui");
thread->window->update();
thread->window->unlock_window();
}
367
368 }
- update_gui will be macro in the future
370
371 void C41Effect::render_gui(void* data)
372 {
373 if(thread)
374 {
375 if(load_configuration()){
376 thread->window->lock_window();
377
386
387 // Updating the GUI itself
388
thread->window->active->update(config.active);
389
thread->window->compute_magic->update(config.compute_magic);
390
391 thread->window->min_r->update(config.min_r);
392 thread->window->min_g->update(config.min_g);
393 thread->window->min_b->update(config.min_b);
394
thread->window->magic4->update(config.magic4);
395
thread->window->magic5->update(config.magic5);
396
thread->window->magic6->update(config.magic6);
397
398
thread->window->fix_min_r->update(config.fix_min_r);
399
thread->window->fix_min_g->update(config.fix_min_g);
400
thread->window->fix_min_b->update(config.fix_min_b);
401
thread->window->fix_magic4->update(config.fix_magic4);
402
thread->window->fix_magic5->update(config.fix_magic5);
403
thread->window->fix_magic6->update(config.fix_magic6);
Create C41Window::update() and move all updates there
404
405 // DEBUG
406 // printf("UPDATING GUI with RENDER and
values %f %f %f\n", config.min_r,
Remove all debug messages
407 free(data);
Do not use dynamic allocations unless explictly needed
408
409 thread->window->unlock_window();
410 }
411 }
412
413 }
414
415 int C41Effect::load_defaults()
416 {
424 config.min_r = defaults->get("MIN_R", config.min_r);
425 config.min_g = defaults->get("MIN_G", config.min_g);
426 config.min_b = defaults->get("MIN_B", config.min_b);
427 config.magic4 = defaults->get("MAGIC4", config.magic4);
428 config.magic5 = defaults->get("MAGIC5", config.magic5);
429 config.magic6 = defaults->get("MAGIC6", config.magic6);
As min_r...magic6 are always computed and never changed by user there is
no point to save/load them
430
513
514 // Computes the difference (in nanoseconds) between two timespec
515 double C41Effect::difftime_nano(timespec start, timespec end)
516 {
526 }
Do not leave debug functions into release
528 /* Faster pow() approximation; borrowed from
http://www.dctsystems.co.uk/Software/power.html
529 * Tests on real-world data showed a max error of 4% and avg. error
or .1 to .5%,
530 * while accelerating rendering by a factor of 4.
531 */
532 float C41Effect::log2(float i)
533 {
534 return(log(i)/log(2));
535 }
Remove unused functions
537 float C41Effect::myLog2(float i)
538 {
539 float x, y, LogBodge=0.346607f;
540 x=*(int *)&i;
541 x*= 1.0/(1<<23); //1/pow(2,23);
542 x=x-127;
543
544 y=x-floorf(x);
545 y=(y-y*y)*LogBodge;
546 return x+y;
Use spaces around operators, example: c = a + (b + c)
547 }
562 float C41Effect::myPow(float a, float b)
563 {
564 return myPow2(b*myLog2(a));
565 }
566
567
568
No more than 2 empty lines
569 int C41Effect::process_buffer(VFrame *frame,
570 int64_t start_position,
571 double frame_rate)
572 {
573 load_configuration();
574
575 read_frame(frame,
576 0,
577 start_position,
578 frame_rate,
579 get_use_opengl());
Drop get_use_opengl, if you do not support OpenGL
580
581 int frame_w = frame->get_w();
582 int frame_h = frame->get_h();
583
584 switch(frame->get_color_model())
585 {
586 case BC_RGB888:
587 case BC_YUV888:
588 case BC_RGBA_FLOAT:
589 case BC_RGBA8888:
590 case BC_YUVA8888:
591 case BC_RGB161616:
592 case BC_YUV161616:
593 case BC_RGBA16161616:
594 case BC_YUVA16161616:
595 return 0; // Unsupported
596 break;
No need for break
597 case BC_RGB_FLOAT:
598 break;
599 }
600
601 float magic1, magic2, magic3, magic4, magic5, magic6;
602
603 // Profiling
604 timespec start,middle,end;
605 clock_gettime(CLOCK_REALTIME, &start);
No profiling in release
606
607 if(config.compute_magic){
608
609 // Box blur!
610 VFrame* tmp_frame = new VFrame(*frame);
611 VFrame* blurry_frame = new VFrame(*frame);
612
613 float** rows = (float**)frame->get_rows();
614 float** tmp_rows = (float**)tmp_frame->get_rows();
615 float** blurry_rows =
(float**)blurry_frame->get_rows();
616 for(int i = 0; i < frame_h; i++)
617 for(int j = 0; j < (3*frame_w); j++)
618 blurry_rows[i][j] = rows[i][j];
Allocate everything once if needed and delete in destructor
619
620 int boxw = 5, boxh = 5;
621 // 3 passes of Box blur should be good
622 int pass,x,y,y_up,y_down,x_right,x_left;
Spacing: int pass, x, y, y_up, y_down, x_right, x_left;
636
component=(tmp_rows[y_down][x_right]
637
+tmp_rows[y_up][x_left]
638
+tmp_rows[y_up][x_right]
639
+tmp_rows[y_down][x_right])/4;
640 //if( i%250==1 && j%250==1
) printf("values, i=%d j=%d tot
Debug print
641 blurry_rows[y][x]=
component;
642 }
643 }
644 }
663 if(row[0]<minima_r) minima_r =
row[0];
Spacing: if(row[0] < minima_r)
677
678 // DEBUG
679 //printf("Minima: %f %f %f\n", minima_r, minima_g,
minima_b);
680 //printf("Maxima: %f %f %f\n", maxima_r, maxima_g,
maxima_b);
Remove debug messages, please
685 magic5 = log(maxima_g/minima_g) /
log(maxima_r/minima_r);
686 magic6 = log(maxima_b/minima_b) /
log(maxima_r/minima_r);
As below is used 1 / magic{5,6} the division can be reverted
687 // DEBUG
688 // printf("Magic values: %f %f %f\n", magic4,
magic5, magic
Debug messages
690 // Update GUI
Spaces at the end of line
691 float *nf_vals = (float*) malloc(6*sizeof(float));
Why allocation? Use float nf_vals[6] in class data
This causes memory leak, when plugin runs and gui is inactive.
696
697 clock_gettime(CLOCK_REALTIME, &middle);
Profiling subsystem - should be removed
725
726 clock_gettime(CLOCK_REALTIME, &end);
Profiling
727
728 // Profiling
729 // printf("Time elapsed: %f (computing magic values), %f
(processing image), %f total\n",
730
Out commented debug messages
731 /* END NEGFIX */
732
733 return 0;
734 }
735
736 int C41Effect::handle_opengl()
737 {
738 #ifdef HAVE_GL
739 /* TODO */
740 /* NOT IMPLEMENTED */
741 /* TODO */
742 #endif
743 }
No need, if opengl is unsupported here
Einar
