Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Function to serialize compressed image to buffer #139

Merged
merged 8 commits into from
Apr 6, 2021

Conversation

mjcarroll
Copy link
Contributor

Signed-off-by: Michael Carroll [email protected]

@github-actions github-actions bot added 🏢 edifice Ignition Edifice 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint 🔮 dome Ignition Dome labels Dec 14, 2020
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mind adding a quick test to check the buffer's contents?

iche033
iche033 previously approved these changes Feb 20, 2021
Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just minor comment on style otherwise looks good to me

graphics/src/Image.cc Outdated Show resolved Hide resolved
@iche033 iche033 dismissed their stale review February 20, 2021 04:41

just noticed that a test is requested

@chapulina chapulina removed the 🏢 edifice Ignition Edifice label Feb 22, 2021
@nkoenig nkoenig self-requested a review March 1, 2021 21:19
Signed-off-by: Nate Koenig <[email protected]>
@codecov
Copy link

codecov bot commented Mar 22, 2021

Codecov Report

Merging #139 (94f837d) into ign-common3 (6a12f59) will decrease coverage by 0.07%.
The diff coverage is 0.00%.

❗ Current head 94f837d differs from pull request most recent head 152d289. Consider uploading reports for the commit 152d289 to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           ign-common3     #139      +/-   ##
===============================================
- Coverage        75.24%   75.16%   -0.08%     
===============================================
  Files               72       72              
  Lines            10227    10237      +10     
===============================================
  Hits              7695     7695              
- Misses            2532     2542      +10     
Impacted Files Coverage Δ
graphics/src/Image.cc 65.00% <0.00%> (-2.83%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a12f59...152d289. Read the comment docs.

@iche033
Copy link
Contributor

iche033 commented Mar 22, 2021

windows build caught a compile error:

C:\Jenkins\workspace\ignition_common-ci-pr_any-windows7-amd64\ws\ign-common\graphics\src\Image.cc(155): error C2664: 'BOOL FreeImage_AcquireMemory(FIMEMORY *,BYTE **,DWORD *)': cannot convert argument 3 from 'unsigned int *' to 'DWORD *'

@nkoenig
Copy link
Contributor

nkoenig commented Apr 6, 2021

@mjcarroll and @iche033 , CI is now green.

@mjcarroll
Copy link
Contributor Author

@chapulina there isn't an easy way to test this without implementing the inverse (png buffer to image). Since this is needed by subt, can we land it and I'll add that test in a followup?

@chapulina
Copy link
Contributor

Sure sounds good. I was thinking of a simpler test, maybe just checking the size of the buffer. To make sure that the function is exercised in the tests. But don't block on this.

@mjcarroll mjcarroll merged commit 48c64a0 into ign-common3 Apr 6, 2021
@mjcarroll mjcarroll deleted the mjcarroll/image_to_buffer branch April 6, 2021 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 blueprint Ignition Blueprint 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants