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

Allow video scrapers/NFO files/library import to add any artwork type… #12612

Merged
merged 1 commit into from Jan 10, 2018

Conversation

@rmrector
Copy link
Contributor

commented Aug 2, 2017

… for items

Description

Scrapers (along with friends NFO files and library importing from single file XML export) can set any type of artwork (clearlogo, landscape, etc) for items in the video library.

This is such a simple loop change that doesn't disturb the beast that is scrapers, I so very much wish I had found it a couple of years ago when I first went looking for it.

Motivation and Context

The video library has long supported freely named artwork for items in most other places, now scrapers can set this artwork from the beginning.

How Has This Been Tested?

By hand, I modified the new demo Python scrapers and a classic scraper to set some extra artwork, and modified a couple of NFO files to match, then ensuring the results are what I expect. I also refreshed a few items with a regular scraper and ensured they still set their expected artwork.

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the Code guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the CONTRIBUTING document
  • I have added tests to cover my change
  • All new and existing tests passed
@rmrector

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2017

The build failure for Win-64 looks like it was caused by something else.

@mkortstiege mkortstiege requested review from xhaggi and hudokkow Aug 3, 2017
@hudokkow

This comment has been minimized.

Copy link
Member

commented Aug 3, 2017

Sorry, this is not my territory.
jenkins failure indeed seems unrelated. How the hell do I remove my name from review request list?

EDIT: Silly me. Removed myself from reviewers.

@hudokkow hudokkow removed their request for review Aug 3, 2017
@rmrector

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2017

Anyone have any thoughts on this? This is kind of important, a lot of folks like extended artwork and this has been a big hole in artwork support for awhile now (relying on an add-on seems unnecessary, scrapers can add some artwork so they should be able to add all of it).

One hole is left, Kodi doesn't import extended artwork from image files next to media. I've got Python code that identifies these images just fine, but for some reason I just cannot translate it to Kodi core (C++ noob, gah!).

@zag2me

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2017

@DaveTBlake was looking at imports and exports recently

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Aug 17, 2017

For music, scraping of video is different, but I am interested in how we can handle art better in core rather than leave it to addons. There is also #11755, which could be of interest.

@rmrector

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2017

@phate89 You've been hard at work on bigger parts of the scrapers, what do you think of this? This code sits up a level higher, as a filter on the data from the scraper before it makes it into the database, and is still useful to your open PR #12661.

@MartijnKaijser With all your work on Artwork Downloader, what do you think of scrapers being able to add more artwork?

@@ -1397,6 +1378,18 @@ namespace VIDEO
art.insert(std::make_pair("fanart", fanart));
}

// add online art
std::vector<CScraperUrl::SUrlEntry>& scraperArt = pItem->GetVideoInfoTag()->m_strPictureURL.m_url;
for (auto i = scraperArt.begin(); i != scraperArt.end(); ++i)

This comment has been minimized.

Copy link
@phate89

phate89 Sep 5, 2017

Contributor

You could use a range-based for like this to make it simpler
for (const auto& i : pItem->GetVideoInfoTag()->m_strPictureURL.m_url)

@phate89

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2017

I'm not an expert of artwork code but it seems fine and also simpler. Why did you move the loop after fanart gathering?

@rmrector rmrector force-pushed the rmrector:the_art_of_scrape branch from 14a9e71 to f489c22 Sep 6, 2017
@rmrector

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2017

I moved the loop because it can also provide a "fanart" image, but the original fanart section still offers an extra feature, the rarely-used fanart colors for skins, so they should take priority.

I just added a bit that covers what the original art.empty() check did for XML scrapers: it considers artwork with an empty aspect to be "thumb", which is a reasonable assumption and matches scraper behavior for episodes, or "poster" for music videos, which is a quick hack to support the XML music video scrapers that share scraper bits with music album scrapers.

@zag2me

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2017

Does this work for music scrapers also?

@phate89

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2017

@rmrector I really don't know anything but I always tought that fanart was only in m_fanart..
We explicitely drop it from arTypes here https://github.com/rmrector/xbmc/blob/f489c22bd92aa374c17b0de62ccaef946c57236f/xbmc/video/VideoInfoScanner.cpp#L1338
But I think it make sense to be able to provide all artwork from one place.. maybe @xhaggi know better
@zag2me these are only video changes. No idea if the same could be done in music part

@rmrector rmrector force-pushed the rmrector:the_art_of_scrape branch from f489c22 to 2f12f40 Sep 7, 2017
@rmrector

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2017

@zag2me The music library doesn't support the same wide selection of artwork as the video library in the first place, so it can't be extended this simply and is outside the scope of this PR.

@phate89 This new code doesn't use artTypes when processing scraper artwork (artTypes is what held back the original code), but accepts the first image of each art type/"aspect" the scraper provides, so if the scraper sets an image aspect to "fanart" in addAvailableThumb then it will show up here. I agree that all artwork should work the same, so I didn't want to add an exception to ignore "fanart" here, and I don't want to disturb XML scrapers, so I kept the existing m_fanart as well. If it weren't for that rarely-used feature of fanart, the colors (I like the idea, but I think they should be attached to the TV show and occasionally season rather than each fanart), I'd say Python scrapers shouldn't even have a setAvailableFanart. The only web service that has that info is TheTVDB, and their new v2 API (v1 will disappear soon) doesn't seem to provide the color anymore (nor their beta website), so maybe it should even go away anyway?

I've also push another change to handle season artwork, which is in another section of code. One thing about season artwork is that it doesn't work from Python scrapers in master (with notspiff's initial Python scraper), but does look to be addressed in your PR. I haven't given it a test yet, but I intend to soon; the scraper process has some goofiness with artwork, and I think the Python scraper interface can smooth some of it out.

Edit: And the WIN64 build failure looks unrelated, the rest compiled fine.

@rmrector rmrector force-pushed the rmrector:the_art_of_scrape branch from d53c176 to 902eb79 Dec 20, 2017
Copy link
Contributor

left a comment

fix the very minors and i will merge this. sorry for the long wait!

{
thumb = CScraperUrl::GetThumbURL(pItem->GetVideoInfoTag()->m_strPictureURL.GetFirstThumb(type));
if (!thumb.empty())
if (thumb.find("http://") == std::string::npos &&

This comment has been minimized.

Copy link
@notspiff

notspiff Dec 21, 2017

Contributor

please update for the new world of https everywhere. and use StringUtils::StartsWith

This comment has been minimized.

Copy link
@rmrector

rmrector Dec 22, 2017

Author Contributor

The find in the line below catches https:// and every other string that has a protocol, this line is unnecessary altogether. I can remove this line and combine the two nested ifs, but I don't see a "Find"-like method in the StringUtils. Is there one somewhere?

This comment has been minimized.

Copy link
@notspiff

notspiff Dec 22, 2017

Contributor

StringUtils::Contains(..) but really using find for stuff anywhere in string is fine by me. It was the anchoring to front that was most important in my request and startswith conveys that a bit quicker to the reader of code.

@rmrector rmrector force-pushed the rmrector:the_art_of_scrape branch from 902eb79 to a61e3e5 Dec 22, 2017
@rmrector

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2018

@notspiff ping.

@xhaggi

This comment has been minimized.

Copy link
Member

commented on xbmc/video/VideoInfoScanner.cpp in a61e3e5 Jan 10, 2018

It would be nice if we could avoid using i for range based for loops. Name it url, this makes it clear in the scope of the loop.

@notspiff

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2018

jenkins build this please

@notspiff

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2018

@xhaggi i will do a follow up with the cosmetics, let's get this in now.

@notspiff notspiff merged commit f444a90 into xbmc:master Jan 10, 2018
1 check passed
1 check passed
default You're awesome. Have a cookie
Details
@olympia

This comment has been minimized.

Copy link

commented Jan 10, 2018

Cool!
Would someone explain to me (who is unable to read the code) what to change / how to use this in the scrapers?

@notspiff

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2018

you have an 'aspect' tag on the entry that can be "poster", "thumb", etc.

@Rechi Rechi added this to the L 18.0-alpha1 milestone Jan 10, 2018
@rmrector rmrector deleted the rmrector:the_art_of_scrape branch Feb 20, 2018
rmrector added a commit to rmrector/xbmc that referenced this pull request Feb 22, 2019
Small adjustment to xbmc#12612 to keep backward-compatibility with
NFOs exported from Kodi 11 Eden scrapers.
rmrector added a commit to rmrector/xbmc that referenced this pull request Feb 24, 2019
Small adjustment to xbmc#12612 to keep backward-compatibility with
NFOs exported from Kodi 11 Eden scrapers.
@rmrector rmrector referenced this pull request Feb 24, 2019
3 of 13 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.