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

[gif] - add gif support via giflib #5180

Closed
wants to merge 9 commits into from
Closed

Conversation

Memphiz
Copy link
Member

@Memphiz Memphiz commented Aug 7, 2014

This is a split off from #5166

This removes the old animatedgif impl. I was not able to test if this introduces regressions in any way. I just don't know how to trigger that old animatedgif code to verify that it still works.

We use giflib5 for depends and windows - but the implementation also supports the giflib4 api.

@notspiff
Copy link
Contributor

notspiff commented Aug 7, 2014

thx...

@Memphiz
Copy link
Member Author

Memphiz commented Aug 8, 2014

Updated - @jmarshallnz , @t-nelson and @notspiff - the commits are suffixed with your nicks for better review.

Only unsure about the endianess @t-nelson - as i see @ace20022 copied the makro from here:

http://sourceforge.net/p/giflib/code/ci/master/tree/lib/dgif_lib.c#l27

so seems there is no be support inside the lib itself no?

@Memphiz
Copy link
Member Author

Memphiz commented Aug 8, 2014

Also the loopedonce bool was split into its own commit and moved to the other pr about animated splash ...

@@ -115,7 +115,6 @@ CGUITextureBase::CGUITextureBase(const CGUITextureBase &right) :
m_diffuseScaleV = 1.0f;

m_currentFrame = 0;
m_frameCounter = (unsigned int) -1;
m_currentLoop = 0;

This comment was marked as spam.

@Memphiz
Copy link
Member Author

Memphiz commented Aug 9, 2014

crap and with that m_Frames change it doesn't work anymore ... well thats it for some days from me ... nice weekend :)

@jmarshallnz
Copy link
Contributor

Enjoy the sun - we've reviewed within the window, so all good once you get back to it to merge (i.e. no rush)

@Memphiz
Copy link
Member Author

Memphiz commented Aug 9, 2014

could you elaborate on the smart_ptr approach a bit - i guess you mean boost::shared_ptr <- std::vectorboost::shared_ptr doesn't change anything - i bet i didn't get the concept yet ;)

@jmarshallnz
Copy link
Contributor

Yeah.

typedef boost::shared_ptr<GifFrame> FramePtr
std::vector<FramePtr> m_frames;

Then when you add a frame use:

FramePtr frame(new GifFrame(foo, bar));
frame->foo = bar;
m_frames.push_back(frame);

The copy constructor etc. can stay if you like (or alternatively make the definition, assignment etc. private so that no copy construction can be done?) but will be unused, as the vector will just be twiddling pointers when it resizes etc.

Basically it's just a nicer version of std::vector<GifFrame*> where you don't need to care about deletion when the vector goes away.

@Memphiz
Copy link
Member Author

Memphiz commented Aug 9, 2014

updated

@ace20022
Copy link
Member

@Memphiz Can I help? The current situation is a bit confusing, what's the "head revision"?

@Memphiz
Copy link
Member Author

Memphiz commented Aug 11, 2014

@jmarshallnz updated - sorry i'm not really used to the boost stuff ;)
@ace20022 well i guess after the review is through i will squash this down to "your" 2 commits (the first 2 in this PR). After that i will rebase the animatedsplash PR ontop of that and we may start working on getting that right (same for your branch - you might want to rebase to mine as that gif implementation was reviewed and altered then which makes it "newer" then yours). Basically we just split it up in smaller peaces a bit ... divide and conquer ;)

@Memphiz
Copy link
Member Author

Memphiz commented Aug 11, 2014

@jmarshallnz one last thing - i didn't change the COLOR pointer as i don't feel comfortable with using a 4 char array there. It would break type safety and a ptr doesn't hurt imo.

@notspiff
Copy link
Contributor

but obviously that did nothing for the peace of mind of ace. sorry couldnt resist ;)

@Memphiz
Copy link
Member Author

Memphiz commented Aug 11, 2014

wat?

@notspiff
Copy link
Contributor

just a PIECE of bad humor

#endif
if (!m_gif)
{
#if GIFLIB_MAJOR == 4

This comment was marked as spam.

@jmarshallnz
Copy link
Contributor

My version of the branch has fixes for COLOR, fixes the v4/v5 issue, and also some minor warnings fixes. Top 3 commits.

https://github.com/jmarshallnz/xbmc/tree/gifsupport3

Still need to check v5 build (doing so atm)

@jmarshallnz
Copy link
Contributor

Top 5 commits should do it.

@Memphiz
Copy link
Member Author

Memphiz commented Aug 12, 2014

@jmarshallnz squashed it down (and your 5 commits into it aswell). Thats not meaning that i am not willing to do any more changes - i just wanted to get rid of the big commit count so that i don't have to do one big rebase at the end ...

@ace20022
Copy link
Member

@Memphiz version built. While adapting the code, I've seen some other things I want to change. I'll try to do a pr against your branch tonight.

@Memphiz
Copy link
Member Author

Memphiz commented Aug 12, 2014

PRs done in the night always have some excitement - go on :D

@MartijnKaijser MartijnKaijser removed this from the Pending for inclusion milestone Jun 14, 2015
@MartijnKaijser
Copy link
Member

@Memphiz @fritsch is this obsolete when we wikll switch to ffmpeg?

@Memphiz
Copy link
Member Author

Memphiz commented Dec 24, 2015

I have no idea

@fritsch
Copy link
Member

fritsch commented Dec 24, 2015

Nope - Gifs are handled separately, so this PR is not obsolete.

@fritsch
Copy link
Member

fritsch commented Dec 24, 2015

@ace20022 can you also tell your 50 cents? :-) thanks much

@ace20022
Copy link
Member

Superseded by #7960

@razzeee razzeee closed this Dec 24, 2015
@Memphiz Memphiz deleted the gifsupport3 branch May 22, 2016 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants