Hello Einar,
Sorry about taking so long to integrate your feedback. The amended code is v0.2
ob GitHub <https://github.com/Pluies/C41/tree/v0.2>.
I addressed all the issues you mentioned, apart from three which I'm unsure
about:
> As I understand there is no need for c41.h and c41window.h
Those headers files are needed because the two files are coupled —
obviously the plugin needs to access its window, but the window and its
thread also need to know about C41main. I don't see how to delete those
headers without moving all the code to c41.C, which would make it a way
longer and harder to read file. My knowledge of linking in C and C++ is
quite rusty, so if you know of a better way to achieve this, let me know
and I'll implement it.
> *Allocate everything once if needed and delete in destructor*
The matrices I get there have to be of the frame size, and as far as I know
the frame size isn't known at instantiation time, it depends on the frame
passed to process_buffer.
> *Use BC_Title instead of C41TextBox, to reflect the fact that min_r,
min_g... are unchangeable*
On purely subjective grounds, I think it makes more sense visually to have
an unmodifiable TextBox instead of a changing "title". That way, the user
can see immediately that these variables must be computed somehow, whereas
having them as a title could lead to thing they're static.
Looking forward to hearing from your feedback.
Regards,
Florent Delannoy
On 31 December 2011 11:33, Einar Rünkaru <eina...@smail.ee> wrote:
> 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
>
