[Cores] Spring cleaning #2444

Merged
29 commits merged into from Apr 5, 2013

Projects

None yet

9 participants

@ace20022
Member

This time I Cppcheck-ed the whole cores folder.

@ace20022
Member

Implemented @cptspiff suggestions. Can you check if it's OK so: 46e221db883ecc259f5611bfd422eb7e3d56415b

@akva2
Contributor
akva2 commented Mar 18, 2013

it's what i had in mind. if it still compiles, it's all good.

@akva2
Contributor
akva2 commented Mar 18, 2013

argh, wrong id, ^^ cptspiff at work.

@ace20022
Member

yes it does, thanks.

@ghost
ghost commented Mar 30, 2013

rebase

@ace20022
Member

Will do tomorrow morning. If this is ready to merge already, I will remove the tags while rebasing. (?)

@ace20022
Member

@cptspiff rebased.

@akva2
Contributor
akva2 commented Apr 5, 2013

thanks squash it up and i'll hit the green and shiny.

@ace20022
Member
ace20022 commented Apr 5, 2013

Just to be sure, squash the most recently with their "partners" or squash all 31 commits into one?

@jmarshallnz
Member

The former.

ace20022 added some commits Mar 11, 2013
@ace20022 ace20022 [DVDOverlay] Correct comments. 1fc1595
@ace20022 ace20022 [DVDOverlay] Correct the loop in the OverlayGroup copy constructor. 07430e3
@ace20022 ace20022 [DVDPlayerAudioResampler] Check for realloc failures, NO free() calls. 7afd4de
@ace20022 ace20022 [CrytalHD/OpenMax] Fix "Common realloc mistake: 'out' nulled but not …
…freed upon failure".
e32f111
@ace20022 ace20022 [Cores] Initialize members in the constructors. 92e8bc4
@ace20022 ace20022 [EmuFileWrapper] Remove unused member variable m_initialized. c43f5ba
@ace20022 ace20022 [DVDStreamInfo] Complete Assign(...) method. 85df0e2
@ace20022 ace20022 [DVDSubtitleTagSami] Possible leak in public function. f54dbb0
@ace20022 ace20022 [Cores] Make LibraryLoader and CConvolutionKernel non-copyable. 7a8a734
@ace20022 ace20022 [Cores] Performance: pre-increment/decrement instead of post-incremen…
…t/decrement.
6152735
@ace20022 ace20022 [Cores] Performance: use empty() instead of size() where appropriate. db67c5e
@ace20022 ace20022 [WinRenderer] Remove redundant assignments. c2fd5d8
@ace20022 ace20022 [Cores] Remove redundant assignments. 658ef93
@ace20022 ace20022 [Cores] Use initialization list. 666cf54
@ace20022 ace20022 [Cores] Remove unused structs and struct members. 64089f0
@ace20022 ace20022 [BaseRenderer] Remove unused var newWidth. cff8eb7
@ace20022 ace20022 [Cores] Remove unused vars. 1f73903
@ace20022 ace20022 [MP3codec] Remove unused var bpf and a switch block manipulation it. 24c0439
@ace20022 ace20022 [Cores] Reduce scope of variables. 3b9014d
@ace20022 ace20022 [Cores] Remove redundant predicates in conditions. e575a7f
@ace20022 ace20022 [PlayerCoreFactory] Fix 'CDVDStreamInfo::operator=' should return 'CD…
…VDStreamInfo &'.
b9d0c4d
@ace20022 ace20022 [DVDStreamInfo] Discuss: 'CDVDStreamInfo::operator=' should return 'C…
…DVDStreamInfo &'. See comment in code.
5dc64ae
@ace20022 ace20022 [Cores] Add constructors where recommended. 34e486d
@ace20022 ace20022 [WinRenderer] Performance: change order of predicates in condition. 6dbf6fb
@ace20022 ace20022 [OMXVideo] Same expression on both sides of '-'. Set it directly to z…
…ero.
ee942ec
@ace20022 ace20022 [Cores] Fix possible NULL pointer dereferences. 0914fd7
@ace20022 ace20022 [Cores] Performance: use static functions if possible. 7258bf7
@ace20022 ace20022 [Cores] Order init lists correctly. 712c3f8
@ace20022 ace20022 [OMXPlayer] Performance: Remove unnecessary NULL pointer checks. 8569e42
@ace20022
Member
ace20022 commented Apr 5, 2013

@cptspiff done ;)

@ghost ghost merged commit 31a41b8 into xbmc:master Apr 5, 2013
@ace20022 ace20022 deleted the ace20022:cleanup_cores branch Apr 8, 2013
@ace20022
Member

@cptspiff I just found this by re-checking the folder with a new version of cppcheck. It was really foolish on my part to commit it this way.
What do think about the warning?

Contributor
akva2 replied Apr 8, 2013

damm, i forgot about this one. operator= should indeed return a reference so we can do stuff like a = b = c.

@manio
Contributor
manio commented on e575a7f Apr 10, 2013

Hello
This commit broke playing PVR radio channels for me.
I've got in the log an assert:

*__GI___assert_fail (assertion=0x10ce200 "m_iSourceChannels > 0 && m_iSourceChannels < 3", file=, line=220, function=0x10ce280 "virtual CAEChannelInfo CDVDAudioCodecLibMad::GetChannelMap()") at assert.c:81

Full debug: http://skyboo.net/xbmc/xbmc_crashlog-20130410_190044.log

After reverting it - it plays fine.

Member

I've opened a pr here: #2588. I'm really sorry!

Contributor

np, thank you for a quick fix :)

@popcornmix

if m_tAudio=-1 and item.IsAudio()=1 the old code continues, then new code returns.

Member

Damn operator precedence and unreadable conditions, tricked me and cppcheck...

Member

@cptspiff Revert the whole block?

Contributor

I'd revert the entire block to fix the breakage and then try again with another PR.

@FernetMenta
Member

backport to Frodo? I think this fixes trac 14334 (Rapidly changing live TV channel crashes XBMC)

Member

If someone could create a patch that applies cleanly on top of the latest Frodo branch I could try it to see if it fixes the issue. I don't have enough git-fu to do it myself.

Member

not sure if it applies cleanly:
git checkout -b Frodo origin/Frodo ; git show 92e8bc4a4361d730abac9ad3080cd6923e9d551a | git apply

edit:
as @cptspiff pointed out, i was sleepy when i wrote this i guess, git cherry-pick would have done the trick too :)

Member

@opdenkamp some parts of the patch won't apply:

sam@gorbachov:/usr/src/xbmc$ git reset --hard
HEAD is now at 32b1a5e bump version to 12.2
sam@gorbachov:/usr/src/xbmc$ git show 92e8bc4 | git apply
:286: trailing whitespace.

:485: trailing whitespace.
m_flush = false;
error: patch failed: xbmc/cores/dvdplayer/DVDCodecs/Video/DVDVideoCodecVDA.cpp:208
error: xbmc/cores/dvdplayer/DVDCodecs/Video/DVDVideoCodecVDA.cpp: patch does not apply
error: patch failed: xbmc/cores/omxplayer/OMXVideo.cpp:86
error: xbmc/cores/omxplayer/OMXVideo.cpp: patch does not apply
sam@gorbachov:/usr/src/xbmc$

I modified the patch to leave those two untouched then recompiled and it seems to (at least partially) fix #14334. More info on trac.

Contributor

@all, I won't accept a shotgun attempt as a backport to Frodo, find which bits really fix trac 14334 and that I will accept.

Member

This whole commit is a fix so I don't see why backporting would be an issue. I don't have time to do more testing today but I could try to pin-point which file needs patching (I suspect xbmc/cores/dvdplayer/DVDCodecs/Video/VDPAU.cpp) tomorrow.

Member

it's definitely a good idea to have this backported to frodo

Contributor

It's a problem in that it introduces possible unknown issues that might cause regressions. This is not a fix that is aimed at fixing a specific issue. This is a shotgun patch and I don't accept shotgun patches this late in the release schedule. Since I am the Release Manager for Frodo, I am in control of what goes in and I've already given my thoughts on this.

Member

sure, you are in control of the release, and i respect that. i was just giving my thoughts about this one.

if something in frodo depends on uninitialised variables with random content, and we break that by assigning them in constructors so mem is no longer uninitialised, then that would be very very odd ;-)

Member

That couple of lines in vdpau.cpp are the interesting part for trac 14334:

+  past[0] = NULL;
+  past[1] = NULL;
+  current = NULL;
+  future = NULL; 
Contributor

yes, very very odd but not impossible and I'm not risking a release to find out.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment