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

[PVR] Recordings: Prevent thumbnail extraction (as it cannot work pro… #10373

Merged
merged 2 commits into from Sep 2, 2016

Conversation

ksooo
Copy link
Member

@ksooo ksooo commented Aug 31, 2016

…perly without major pvr addon api changes).

Fixes the ugly problem reported here: #10333 (comment)

@Jalle19, @FernetMenta good to go? I think I now found a good place for the "pvr recording check"...

@ksooo ksooo added Type: Fix non-breaking change which fixes an issue Component: PVR v17 Krypton labels Aug 31, 2016
@ksooo ksooo added this to the Krypton 17.0-beta2 milestone Aug 31, 2016
@ksooo
Copy link
Member Author

ksooo commented Aug 31, 2016

@axbmcuser fyi

@@ -86,6 +86,9 @@ bool CThumbExtractor::operator==(const CJob* job) const
bool CThumbExtractor::DoWork()
{
if (m_item.IsLiveTV()

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@ksooo
Copy link
Member Author

ksooo commented Sep 1, 2016

jenkins build this please

@FernetMenta
Copy link
Contributor

Could you wait until Saturday? Why got this broken again? iirc I have fixed this some time ago.

@ksooo
Copy link
Member Author

ksooo commented Sep 1, 2016

iirc I have fixed this some time ago.

@FernetMenta You mean this fix, right? https://github.com/xbmc/xbmc/blob/master/xbmc/cores/VideoPlayer/DVDFileInfo.cpp#L107

Fine, but there are more cases you didn't catch. So, it's still broken, not again, imo. ;-)

There are many places where CVideoThumbLoader instances are created.

CVideoThumbLoader instanciates and starts CThumbExtractors => https://github.com/xbmc/xbmc/blob/master/xbmc/video/VideoThumbLoader.cpp#L417

CThumbExtractor::DoWork not only calls CDVDFileInfo::ExtractThumb (the case you fixed), but also CDVDFileInfo::GetFileStreamDetails => https://github.com/xbmc/xbmc/blob/master/xbmc/video/VideoThumbLoader.cpp#L140

My approach is more restrictive than yours because it generally prevents CThumbExtractor to access pvr recordings, regardless from where it's called. BTW, I did not invent this "type filter" approach at the beginning of CThumbExtractor::DoWork, I just added one case.

EDIT: In case we agree to merge this PR, your abovementioned fix could even be removed, as CDVDFileInfo::ExtractThumb gets exclusively called by CThumbExtractor::DoWork, which now will never call this any more on pvr recordings. So, we would have a perfect filter at a single place.

@FernetMenta
Copy link
Contributor

Thanks for explanation. Go ahead

In case we agree to merge this PR, your abovementioned fix could even be removed

could you do this? maybe move the other exceptions like IsDiscImage to the other place too?

@ksooo
Copy link
Member Author

ksooo commented Sep 2, 2016

@FernetMenta yep, will do.

… not needed any longer, as now done in CThumbExtractor::DoWork()
@ksooo ksooo force-pushed the prevent-recordings-thumbnail-extraction branch from 793fe4d to e52bf39 Compare September 2, 2016 17:44
@ksooo
Copy link
Member Author

ksooo commented Sep 2, 2016

In case we agree to merge this PR, your abovementioned fix could even be removed

could you do this? maybe move the other exceptions like IsDiscImage to the other place too?

@FernetMenta done. IsDiscImage check was at "the other place" already, btw.

@ksooo
Copy link
Member Author

ksooo commented Sep 2, 2016

jenkins build this please

@ksooo
Copy link
Member Author

ksooo commented Sep 2, 2016

Travis error is unrelated

@ksooo ksooo merged commit 5afa5fc into xbmc:master Sep 2, 2016
@ksooo ksooo deleted the prevent-recordings-thumbnail-extraction branch September 2, 2016 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: PVR Type: Fix non-breaking change which fixes an issue v17 Krypton
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants