Page MenuHomePhabricator

evas_generic_loaders: port poppler to the cpp api
ClosedPublic

Authored by bu5hm4n on Jun 13 2016, 4:17 AM.

Details

Summary

This ports the loader to the stable cpp api.

Test Plan

Please test this patch and tell me if there are differences

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
bu5hm4n updated this revision to Diff 9279.Jun 13 2016, 4:17 AM
bu5hm4n retitled this revision from to evas_generic_loaders: port poppler to the cpp api.
bu5hm4n updated this object.
bu5hm4n edited the test plan for this revision. (Show Details)
bu5hm4n added a reviewer: DaveMDS.
bu5hm4n added a subscriber: jayji.
bu5hm4n updated this revision to Diff 9286.Jun 13 2016, 7:34 AM

remove debug output

vtorri edited edge metadata.Jun 13 2016, 10:20 AM

I would put the tests outside the for loops over x and y (for speed)

bu5hm4n updated this revision to Diff 9287.Jun 13 2016, 12:41 PM
bu5hm4n edited edge metadata.

update to vtorris comment

Anything else ? I will merge it in ~12h

  • If there is nothing else **

why checking in configure.ac several versions of poppler ? It is a stable API

Thanks for the patch. I will try it when I'll reach work.
I just have a small remarks: wouldn't it be better to remove IMAGE_PIXEL_ITERATOR_END? It will make the use of the iterator less cumbersome

#define IMAGE_PIXEL_ITERATOR \
   for (y = 0; y < crop_height; y++) \
        for (x = 0; x < crop_width; x++)

Plus, in Eina and stuff, we are using the FOR_EACH terminology, so maybe we could have:

#define IMAGE_FOR_EACH_PIXEL \
   for (y = 0; y < crop_height; y++) \
        for (x = 0; x < crop_width; x++)

IMAGE_FOR_EACH_PIXEL
  {
     do->stuff();
     do->another_stuff();
  }

IMAGE_FOR_EACH_PIXEL
   do->stuff();

Also, we could save some calculations in these loops. I can see the same thing is calculated 2 or 4 times (depending of the code path): x+y*crop_width.
I think at the beginning of the loop, you could do:

const int blah = x + (y * crop_width);

and use blah (I have no inspiration for a meaningful name) then.

instead of blah, maybe this is better :

int iter = 0;

int the loop:

dst[iter] = src[iter];
iter++;

bu5hm4n updated this revision to Diff 9289.Jun 13 2016, 11:34 PM

remove useless checks

Also, src should not be re-assigned at each iteration, must be done before. There is already a scope that can enclose it in both cases.

Actually... the before and after code that copies the pixels seems quite different to me, and changes the previous behaviour? Was there a bug in the previous algorithm?

I mean, before, the iteration was done with: src += width; dst += crop_width; Now, we are affecting both src and dst with crop_width.
Also, using memcpy() is better than doing a copy loop (image::format_argb32).

This revision was automatically updated to reflect the committed changes.