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

support for image decoder add-ons #11086

Merged
merged 4 commits into from
Feb 8, 2017
Merged

support for image decoder add-ons #11086

merged 4 commits into from
Feb 8, 2017

Conversation

notspiff
Copy link
Contributor

@notspiff notspiff commented Dec 7, 2016

This is my work on support for image add-ons. The main motivation for this is bringing back libraw support that was lost when cximage was kicked out.

i have confirmed that decode works. you find addons in my github account (raw only for now). if you want to try the raw stuff https://www.rawsamples.ch/index.php/en/canon is convenient.

@notspiff
Copy link
Contributor Author

notspiff commented Dec 7, 2016

@AlwinEsch hope this doesn't create too much problems for your refactoring effort! it should be straight forward to move to a new API like other stuff. of course, it means more work, but that's the cost of progress ;)

@hudokkow hudokkow added Component: Add-ons RFC PR submitted for gathering feedback v18 Leia WIP PR that is still being worked on Component: Binary add-ons labels Dec 7, 2016
@hudokkow
Copy link
Member

hudokkow commented Dec 7, 2016

Probably should have a new component? kodi-imageencoder-dev? Something for later.

const char* mimetype;
};

struct ImageEncoder

This comment was marked as spam.

This comment was marked as spam.

@notspiff notspiff force-pushed the imageencoders branch 2 times, most recently from 37cff1d to 05d83b9 Compare January 5, 2017 11:11
@notspiff
Copy link
Contributor Author

notspiff commented Jan 5, 2017

due to the underwhelming feedback i have taken a few executive decisions:

  • no support for thumbnail creation in add-ons. decode / encode cycle suffices for these (was only ever relevant for jpeg)
  • the factory isn't hot enough to be worth complicating. i listed a dir with 1000 raw's and the time to instance decoders was utterly irrelevant.
  • kodi-image-dev is a separate package

i thus renamed the extension point kodi.imagedecoder. notspiff/imagedecoder.raw for an add-on.

i also rebased for the new add-on stuff (struct table and such).

as far as i am concerned this is ready for merge.

@notspiff notspiff changed the title support for image (en|de)coder add-ons support for image decoder add-ons Jan 5, 2017
@hudokkow
Copy link
Member

hudokkow commented Jan 5, 2017

CMake changes look good to me. Can't comment on the rest.
You might want to update the wording in kodi-image-dev.txt.in to match https://github.com/xbmc/xbmc/pull/11370/files

Happy New Year!

@@ -911,7 +911,8 @@ bool CWinSystemX11::CreateIconPixmap()
gRatio = vis->green_mask / 255.0;
bRatio = vis->blue_mask / 255.0;

CBaseTexture *iconTexture = CBaseTexture::LoadFromFile("special://xbmc/media/icon256x256.png");
CBaseTexture *iconTexture = CBaseTexture::LoadFromFile("special://xbmc/media/icon256x256.png",
0, 0, "image/png");

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@fritsch
Copy link
Member

fritsch commented Jan 5, 2017

@AlwinEsch as you are currently reworking binary addon interfaces, please also have a look.

Thanks very much for doing that work. Very nice and non intrusive.

@notspiff notspiff force-pushed the imageencoders branch 2 times, most recently from 7f926d5 to 6bfec3a Compare January 5, 2017 12:15
@notspiff
Copy link
Contributor Author

notspiff commented Jan 5, 2017

packaging comment updated.

@Paxxi
Copy link
Member

Paxxi commented Jan 14, 2017

If there's no objections, rebase and merge?

@hudokkow
Copy link
Member

No objections from me.

@hudokkow
Copy link
Member

hudokkow commented Feb 1, 2017

@AlwinEsch, any comments or can this be rebased and merged?

@MartijnKaijser
Copy link
Member

can we just move forward or do we need to wait?

@@ -136,6 +138,8 @@ std::shared_ptr<IAddon> CAddonBuilder::Build()
return CAudioEncoder::FromExtension(std::move(m_props), m_extPoint);
case ADDON_AUDIODECODER:
return CAudioDecoder::FromExtension(std::move(m_props), m_extPoint);
case ADDON_IMAGEDECODER:
return CImageDecoder::FromExtension(std::move(m_props), m_extPoint);

This comment was marked as spam.

This comment was marked as spam.

#define XB_FMT_A8 32
#define XB_FMT_RGBA8 64
#define XB_FMT_RGB8 128
#define XB_FMT_OPAQUE 65536

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@AlwinEsch
Copy link
Member

On the whole it is very good and nice.

@notspiff
Copy link
Contributor Author

notspiff commented Feb 6, 2017 via email

{
if (!Initialized())
return false;

This comment was marked as spam.

@notspiff
Copy link
Contributor Author

notspiff commented Feb 6, 2017 via email

@notspiff
Copy link
Contributor Author

notspiff commented Feb 7, 2017

branch rebased, translation layer added, imagedecoder.raw updated.

@notspiff
Copy link
Contributor Author

notspiff commented Feb 8, 2017

everybody seems happy, can we please merge so i don't have to rebase this again.

@Paxxi Paxxi merged commit 8fdd918 into xbmc:master Feb 8, 2017
@Paxxi
Copy link
Member

Paxxi commented Feb 8, 2017

If anything breaks, I never touched the button! 😀

@AlwinEsch AlwinEsch removed the WIP PR that is still being worked on label Feb 8, 2017
@AlwinEsch
Copy link
Member

Have tested it and works nice.

Found by chance and test a good picture :D :
bildschirmfoto vom 2017-02-08 16-54-35

@fritsch
Copy link
Member

fritsch commented Feb 8, 2017 via email

@Paxxi
Copy link
Member

Paxxi commented Feb 8, 2017

I completely forgot about it, threw together a quick build for libraw
https://github.com/Paxxi/LibRaw builds fine but untested

@notspiff
Copy link
Contributor Author

notspiff commented Feb 8, 2017

pushed a dependency definition in the add-on. untested but should be able kick an add-on build on jenkins to check thinngs.

@notspiff notspiff deleted the imageencoders branch February 8, 2017 19:56
@MartijnKaijser
Copy link
Member

@notspiff
Copy link
Contributor Author

notspiff commented Feb 8, 2017

damn close but somebody else keeps the sigar. mismatch in config file name. it expects RAW-config.cmake, not libraw-config.cmake. needs to be adjusted in either add-on or your repo (idc which).

@Paxxi
Copy link
Member

Paxxi commented Feb 8, 2017

I can fix it tomorrow, you want the whole project/target to be just raw or config enough?

@notspiff
Copy link
Contributor Author

notspiff commented Feb 8, 2017

only care about config file, it needs to match the find rule in the add-on which is currently FindRAW.cmake (config file overrides, i assume you know but just in case).

@Paxxi
Copy link
Member

Paxxi commented Feb 9, 2017

I renamed everything to just raw, would likely just cause issues in the future having different names

@MartijnKaijser MartijnKaijser removed the RFC PR submitted for gathering feedback label Feb 9, 2017
@MartijnKaijser MartijnKaijser added this to the L 18.0-alpha1 milestone Feb 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants