Use system tinyxml library instead of an internal implementation. #619

Merged
merged 4 commits into from May 4, 2012

Projects

None yet
@amejia1
Member
amejia1 commented Jan 11, 2012

This is a continuation of #327
I reset the master branch of my fork and issue 327 was closed, not sure why.

I would like to use system tinyxml for XBMC, at least for Linux and BSD systems. I can't upload XBMC to Debian unless this is fixed (use of an internal implementation of tinyxml is not allowed). There was already some discussion in the last pull request. Did anyone have any further comment on this?

@elupus
Member
elupus commented Jan 11, 2012

I'm not opposed. Just needs testing on windows and osx.

@jmarshallnz
Member

The main comment I made was this one: #327 (comment) referring to the diff you did between tinyxml-2.5.3 and what we have.

Other than number 7 in that list, there was a potential iconv issue in that you're not using the encoding specified in the XML document to do the conversion to UTF8. Compare with https://github.com/xbmc/xbmc/blob/master/lib/tinyXML/tinyxmlparser.cpp#L811

@amejia1
Member
amejia1 commented Jan 11, 2012

Here's the diff again. http://pastebin.com/pTqJcpdG

@amejia1
Member
amejia1 commented Jan 12, 2012

About that number 7, that modification was used to always return a match to any entity passed to GetEntity(). Comments in the library mention that this doesn't work anyway. Looking at the code, it doesn't seem like it would have done anything, since there would have needed to be some kind of mapping between entities and the characters they represent. Below is the mapping I'm referring to.

TiXmlBase::Entity TiXmlBase::entity[ TiXmlBase::NUM_ENTITY ] =
{
{ "&", 5, '&' },
{ "<", 4, '<' },
{ ">", 4, '>' },
{ """, 6, '"' },
{ "'", 6, ''' }
};

Were there other entities we wanted to allow?

@amejia1
Member
amejia1 commented Jan 13, 2012

With regards to the iconv conversion, it seems tinyxml has to be modified directly with the changes made for xbmc. How bad do we need this change?

In the meantime, how about we make using system tinyxml optional, similar to how xbmc can be built with other external libraries?

@jmarshallnz
Member

It doesn't need to be modified directly: You could "simply" read the XML in order to get the XML declaration (and thus encoding) and then use that with your current code to do the iconv conversion.

Cheers,
Jonathan

@elupus
Member
elupus commented Jan 13, 2012

I'm against all optional external libs. Either we run external or not imho.

@davilla
davilla commented Jan 14, 2012

There are some heavy changes going on with AM_ICONV that I'm not comfortable to be doing at this late stage of trying to push out Eden release. I have no clue what the side effects might be and certainly do not think our users should be the b2 gpigs. I don't see how we can even think about a b2 without several weeks of testing.

@jmarshallnz
Member

Why does iconv need to be involved at all here? We have our own charset convertor that handles the iconv stuff for us.

When reading an XML file that we want to convert to UTF8 we "simply":

  1. Read the XML declaration to get the XML encoding (if any). If you have to you can always parse the entire thing first (though one presumes there's a way to just grab the declaration which is the first element anyway).
  2. Do exactly what you're already doing by calling the charset convertor on the entire file - only difference is now you know what the source encoding is.

Assuming that is done, I have no problem with a b2 going out - if it breaks we revert and push b3 immediately.

@amejia1
Member
amejia1 commented Jan 15, 2012

Ok, is this acceptable?

@jmarshallnz
Member

I'd be OK with that.

Needs signoff from all platforms.

@davilla
davilla commented Jan 15, 2012

osx/ios sign off, I'm ok with this now, push it in and let's see if this pig will fly :)

@amejia1
Member
amejia1 commented Jan 15, 2012

Waiting on windows signoff.

@wsoltys
Member
wsoltys commented Jan 15, 2012

Doesn't work out of the box. Had to rename TINYXML_DECLARATION in DECLARATION l171 in XBMCtinyXML.cpp to make it compile with 2.6.2. But I get the following linker errors:

1>tinyxmlSTL.lib(tinyxml.obj) : error LNK2038: mismatch detected for '_ITERATOR_DEBUG_LEVEL': value '0' doesn't match value '2' in SlingboxLib.obj
1>tinyxmlSTL.lib(tinyxmlparser.obj) : error LNK2038: mismatch detected for '_ITERATOR_DEBUG_LEVEL': value '0' doesn't match value '2' in SlingboxLib.obj
1>tinyxmlSTL.lib(tinyxmlerror.obj) : error LNK2038: mismatch detected for '_ITERATOR_DEBUG_LEVEL': value '0' doesn't match value '2' in SlingboxLib.obj
1>     Creating library XBMC\Debug (DirectX)\XBMC.lib and object XBMC\Debug (DirectX)\XBMC.exp
1>GUIDialogSmartPlaylistEditor.obj : error LNK2019: unresolved external symbol "public: void __thiscall TiXmlDocument::operator=(class TiXmlDocument const &)" (??4TiXmlDocument@@QAEXABV0@@Z) referenced in function "public: class CXBMCTinyXML & __thiscall CXBMCTinyXML::operator=(class CXBMCTinyXML const &)" (??4CXBMCTinyXML@@QAEAAV0@ABV0@@Z)
1>Addon.obj : error LNK2001: unresolved external symbol "public: void __thiscall TiXmlDocument::operator=(class TiXmlDocument const &)" (??4TiXmlDocument@@QAEXABV0@@Z)
1>ButtonTranslator.obj : error LNK2019: unresolved external symbol "public: class TiXmlAttribute const * __thiscall TiXmlAttributeSet::Find(class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &)const " (?Find@TiXmlAttributeSet@@QBEPBVTiXmlAttribute@@ABV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@Z) referenced in function "public: int __thiscall TiXmlElement::QueryValueAttribute<class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > >(class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &,class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > *)const " (??$QueryValueAttribute@V?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@TiXmlElement@@QBEHABV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@PAV12@@Z)
1>XBMC\Debug (DirectX)\XBMC.exe : fatal error LNK1120: 2 unresolved externals

Will see if I have more time today to investigate it.

@wsoltys
Member
wsoltys commented Jan 15, 2012

first three errors seems to go away if I use the debug lib of tinyxmlstl. leaves still the unresolved symbols

@wsoltys
Member
wsoltys commented Jan 15, 2012

ok now the end of my monologue ;) The errors are there since I didn't removed our tinyxml sources from the file system.
With the addition of

#define TIXML_USE_STL
#define USE_RENAMED_TIXMLNODE_ELEMENTS
#pragma comment(lib, "tinyxmlSTL.lib")

I get XBMC compiled and running fine. Although I get a lot of redefinition warnings due to #define TEXT TINYXML_TEXT and TEXT is already defined in the M$ includes. Does anyone know if its safe to just ignore this warning or is it better to undef TEXT somewhere?

@wsoltys
Member
wsoltys commented Jan 15, 2012

just for the record. win32 needs some love after merging the pull request (editing the project files, implement the scripts for downloading the lib, etc). I'll happy to do that but I'm on a business trip tomorrow so either do it today or earliest Wednesday.
I would advise removing the tinyxml source from /lib. Otherwise I get problems on win32.

@amejia1
Member
amejia1 commented Jan 15, 2012

I think it's better to fix these warnings. I'll remove tinyxml sources before I push.

@davilla
davilla commented Jan 15, 2012

I'll have to bring tinyxml into osx/ios depends build. Unfortunately, it's just a crappy make file that I'll have to hack up to support cross-compile. Did debian patch theirs ?

@Montellese
Member

Damn I should have checked here first now I duplicated wsoltys' work. I already wrote all the necessary scripts etc and pushed the commit to my repo: Montellese@5046d71

I also had to use the debug version of tinyxmlSTL because of the errors wsoltys mentioned (is the SlingboxLib a lib from us? If that's the case we might be able to get rid of those errors and would be able to use the release version of tinyxml). Furthermore I also get all those warnings concerning TEXT etc so it might be best to first find a solution for this (ideally by removing the USE_RENAMED_TIXMLNODE_ELEMENTS ?).

I didn't have to remove the tinyxml sources from our repo (does osx still use them after this?), I just had to remove them from the Visual Studio project.

@wsoltys
Member
wsoltys commented Jan 15, 2012

nice and yes you should have mentioned that :)
find my repo here https://github.com/wsoltys/xbmc/commits/tinyxml
I use the debug an release version for building and undef TEXT seems to help here.
I already uploaded the precompiled libs to our windeps.
Not removing the sources leads to using the internal tinyxml.h rather than the new one on my setup.

@Montellese
Member

Hehe well at least we are pretty sure we did it right as our solutions are almost identical (and we encountered the same warnings).

After removing the tinyxml folder from the VS project it always used the correct header file on my setup but that's because I put the tinyxml.h file directly into the BuildDependencies/include directory and not in an extra sub-directory so I can use #include <tinyxml.h> instead of #include "tinyxml/tinyxml.h".

@wsoltys
Member
wsoltys commented Jan 15, 2012

and I should stop now pushing to my repo because I do one error after the other ;)

@amejia1
Member
amejia1 commented Jan 15, 2012

@wsoltys I'll merge your changes then.

@wsoltys
Member
wsoltys commented Jan 15, 2012

I had to push another fix for periphal.cpp.
@Montellese: does your lib compile with target release? the debug lib spits a lot of errors and I get one unresolved symbol with the release.lib.

1>Peripheral.obj : error LNK2001: unresolved external symbol "private: static struct TiXmlString::Rep TiXmlString::nullrep_" (?nullrep_@TiXmlString@@0URep@1@A)

@amejia1: you should maybe have a look at periphal.cpp at it seems to use tinyxml directly and not via the wrapper. Could be that after that is fixed all the errors might be gone.

@amejia1
Member
amejia1 commented Jan 15, 2012

@wsoltys Yes I saw some of those includes. I meant to fix those after merging your branch and rebasing from xbmc master.

@amejia1
Member
amejia1 commented Jan 15, 2012

Alright, recheck for windows please.

@davilla
davilla commented Jan 15, 2012

ok, tinyxml is now available as a system lib for osx/ios. Needs to be treated the same as Linux with pkg-check in configure, linkage and includes. Free free to nuke lib/tinyXML as osx/ios will not need it if treated the same as Linux.

@amejia1
Member
amejia1 commented Jan 15, 2012

Ok. To all, tinyxml is removed from xbmc source. I need a signoff on all platforms again.

@wsoltys
Member
wsoltys commented Jan 15, 2012

beside the problem above XBMC compiles fine in debug and release.

@davilla
davilla commented Jan 15, 2012

osx/ios signoff with those three changes, I'll fixup xcode projects for osx/ios/atv2 when it hits trunk.

@davilla
davilla commented Jan 15, 2012

spiff says, these changes to system tinyxml will only work on local files and fail if remote. ie nfo files. can you check this amejia ?

@davilla
davilla commented Jan 15, 2012

osx/ios signoff revoked until we sort this out.

@jmarshallnz
Member

There shouldn't be a problem with non-local files, as everything goes through CFile now, right?

@amejia1
Member
amejia1 commented Jan 15, 2012

@jmarshallnz Yes, everything goes through CFile. I've been able to read config xml files through special://.

@wsoltys
Member
wsoltys commented Jan 15, 2012

please implement the last compile fixes:
wsoltys@4b37cc6

@amejia1
Member
amejia1 commented Jan 15, 2012

@wsoltys @davilla I just merged your changes. Aside from what cptspiff brought up, is there anything else left before I push to xbmc master?

@davilla
davilla commented Jan 15, 2012

not that I can see, osx/ios sign off pending spiffs and the nfo question.

@Montellese
Member

I can confirm that it builds fine on win32 and XBMC runs as usual (as far as I can tell) but I didn't do any special xml-related tests. I won't give a sign off though because off spiffs concerns and because I think it's a very bad idea to change any library between two beta releases (during feature freeze).

@Montellese
Member

I took another look at the TEXT related warnings on win32 and it turns out we use the win32-specific TEXT macro ourselfs in WinSystemWin32.cpp and other places so simply undef'ing it in XBMCTinyXML.h looks very dangerous to me. Can't we just get rid of these and replace their usages with the macros defined by tinyxml.h? I did the work in Montellese@71729ed to see where we use those macros.

@amejia1
Member
amejia1 commented Jan 16, 2012

@Montellese I suppose if all platforms are using tinyxml 2.6.2, then yes, the TINYXML_* enums can be used directly.

@amejia1
Member
amejia1 commented Jan 16, 2012

All, please recheck latest commit for all platforms.

@cptspiff Though I can't get xbmc to read from any nfo files (no calls to LoadFile are made to read an nfo file), I have checked that LoadFile indeed works with special:// and smb:// urls.

@jmarshallnz
Member

A quick grep for SetConvertToUtf8 or convertToUtf8 shows we don't actually use that functionality (it was added by a boxee dev IIRC) so that can be dropped and save the complication of multiple parsing etc.

@wsoltys
Member
wsoltys commented Jan 18, 2012

looks okay for win32.
And I agree with Montellese. The undef in the header was a bad idea (in the cpp it might had worked).

@ghost
ghost commented Jan 18, 2012

other than the two commits i commented on and a general need for some squashing (we don't need the history of you fixing bugs in the initial implementation), it looks good to me.

@jmarshallnz
Member

Rebased version is available here that should address mine and cptspiff's points above:

https://github.com/jmarshallnz/xbmc/commits/system_tinyxml_rebase

Am building/testing on win32. Other platforms please test + give signoff.

@jmarshallnz
Member

Confirmed that the rebased branch above builds and runs on win32.

@amejia1
Member
amejia1 commented Jan 19, 2012

And it builds for me on linux. Shall I merge?

@Pivosgroup

osx/ios sign-off revoked again :)

@jmarshallnz
Member

On reflection, the issue with invalid XML (specifically, XML that includes & but not & ) is unlikely to be fixable properly other than patching tinyxml. Personally, I don't see why tinyxml wouldn't accept a fix - they've obviously considered it in the past - essentially it allows unknown entities to be ignored so that the & isn't stripped.

b2 should go ahead without this.

@davilla
davilla commented Jan 19, 2012

pay no attention to that beard Pivosgroup :) the real osx/ios says revoked :) LOL.

@amejia1
Member
amejia1 commented Jan 19, 2012

Would overriding TiXmlBase::GetEntity be acceptable within XBMC?

@jmarshallnz
Member

Any solution to get it to work would be considered, sure. Alternative is making sure that all entry points for XML into tiny are pre-filtered (atm there's OpenFile, Parse and >> I think?)

@amejia1
Member
amejia1 commented Mar 25, 2012

I'm ready to have this reviewed again.

@amejia1
Member
amejia1 commented Apr 5, 2012

PING PING PING!!!
I'm ready to have this reviewed again. I know it's not mergable right now. I just need an ok from all platforms that this change is good. I can resolve conflicts and merge afterwards.

@jmarshallnz
Member

The changes look reasonable to me - as far as I'm aware there should be no other issues. Mind rebasing so that we can get some builds out for folk to test.

In particular, testing to check that blah blah & foo returns "blah blah & foo" is what's needed. This should be able to be verified by running things through the scraper.

@theuni
Member
theuni commented Apr 6, 2012

Would be nice if a test could be added as well, as I'm sure this will come up again if we decide to shop around xml parsers. Just a dummy function to load/parse/output those specific cases would be very helpful down the road.

@jmarshallnz
Member

Ideally, we'd fix the cases we have invalid data, but yup, agreed. I'll track down the scraper case.

@jmarshallnz
Member

Ok, rebased my branch here: https://github.com/jmarshallnz/xbmc/commits/system_tinyxml_rebase

and added a test. It returns false at the moment as I don't have 39ca75b.

@amejia1 - please pull in my top commit and rebase (or just pull my branch and throw 39ca75b on top).

@amejia1
Member
amejia1 commented Apr 8, 2012

I suppose we need a better way to support these test cases which doesn't involve injecting code into the regular source files.

@amejia1
Member
amejia1 commented Apr 8, 2012

Ping @davilla @wsoltys @jmarshallnz and anyone else following this.

Any objections before this gets merged to master?

@jmarshallnz
Member

I'll pull and run the test on osx later today. It'd be great if someone could run the test on win32 as well

@DDDamian you're on win32, right? If you have a spare half an hour, minding pulling, building and ensuring the test returns true (just dump some test code in CApp::Create)

@DDDamian
DDDamian commented Apr 9, 2012

Had a few fixes to make in vcxproj.filters, then it loaded okay. Did a git clean and reloaded the dependencies, cleaned solution in MS VC++ and tried to build. Linker has an issue with TiXmlBase::GetEntity being defined twice.

1>------ Build started: Project: XBMC, Configuration: Debug (DirectX) Win32 ------
1>tinyxmlSTLd.lib(tinyxmlparser.obj) : error LNK2005: "protected: static char const * __cdecl TiXmlBase::GetEntity(char const *,char *,int *,enum TiXmlEncoding)" (?GetEntity@TiXmlBase@@KAPBDPBDPADPAHW4TiXmlEncoding@@@Z) already defined in XBMCTinyXML.obj
1> Creating library XBMC\Debug (DirectX)\XBMC.lib and object XBMC\Debug (DirectX)\XBMC.exp
1>XBMC\Debug (DirectX)\XBMC.exe : fatal error LNK1169: one or more multiply defined symbols found
========== Build: 0 succeeded, 1 failed, 35 up-to-date, 0 skipped ==========

Code in TinyXML.h and perhaps elsewhere is formatted with tabs as well.

@jmarshallnz
Member

I was afraid of that. The only thing I've found with a quick searchis that something like /FORCE:MULTIPLE might be usable to override the duplicate symbols. Whether that will actually work, I don't know. It seems like a hack at best, and disables things such as incremental linking which seems nasty.

http://msdn.microsoft.com/en-us/library/70abkas3.aspx

@DDDamian: in addition to just building and running, add a call to XBMCTinyXML::Test() in CApp::Create() and ensure it returns true (throw a breakpoint in).

@DDDamian
DDDamian commented Apr 9, 2012

Well, the /Force:Multiple allows the build with warnings, but the Test() failed.

See pic for local vars: http://i40.tinypic.com/2me616s.jpg

@jmarshallnz
Member

No error in the parsing by the looks, and root appears non-NULL, which suggests that url is probably non-NULL as well.

Grab what url->FirstChild() is, in particular, what is url->FirstChild()->Value() ?

@jmarshallnz
Member

Given the hassles here, IMO the only decent solution is to patch tinyxml on win32 (and any other systems that can get away with it).

In the meantime, we should strive to get rid of invalid XML.

The only one that really matters is scrapers, which are fixable, albeit not necessarily easy (XML inside XML never is). If/when they move to python this won't be a problem anymore, so with any luck by the end of the summer the problem will go away of it's own accord.

@DDDamian
DDDamian commented Apr 9, 2012

Odd behaviour indeed.

if (root && root->ValueStr() == "details")
{
TiXmlElement *url = doc.FirstChildElement("url");
if (url && url->FirstChild())
{
return (url->FirstChild()->ValueStr() == "http://api.themoviedb.org/3/movie/12244?api_key=57983e31fb435df4df77afb854740ea9&language=en");
}
}
return false;

First if fails even though root is not null and "details" is present. If I force that if to true, url is still undefined and next if fails.

@wsoltys
Member
wsoltys commented Apr 9, 2012

The vs project file is completely borked and if we even consider merging this it should be overdone. I also don't like the /force given that it worked before when I first tested it. Dunno about the Test(). it seems to be that url is invalid.

@DDDamian
DDDamian commented Apr 9, 2012

My conclusion is the /Force led to dubious behaviour and is asking for more. The project file was easy to remedy, but I'm uncomfortable with /Force as wsoltys mentions. I've left this in my local branch for further testing if a workaround to /Forcing is applied...

@wsoltys
Member
wsoltys commented Apr 9, 2012

I didn't said that's easy or hard it just needs to be done before merging. dunno if this is worth all the hassle and if we shouldn't keep what we have (is it really me saying that? ;).

@amejia1
Member
amejia1 commented Apr 15, 2012

Ok, ready to have this reviewed again by everyone. This passes the test case.

The difference from the other updates is now the data passed to tinyxml is prefiltered to replace the beginning '&' for invalid entities with a '&'.

@amejia1
Member
amejia1 commented Apr 15, 2012

That was an '&' followed by 'amp;'.

@wsoltys
Member
wsoltys commented Apr 16, 2012

Compiles and seems to work fine with the vs filter file change here: wsoltys@aa7b12d.
The test fails as we can't use relative path here (the binary runs somewhere else). Using the right path (special://xbmc/xbmc/utils...) it still doesn't work. I can't explain why. I just see that the special path is transformed into the right local path but File.cpp l240 returns false because CFileFactory::CreateLoader(url) returns a CHDFile() which is m_hFile(INVALID_HANDLE_VALUE).

@jmarshallnz
Member

The fix for the broken XML won't fix those cases where we call Parse() directly from within XBMC. This is what scrapers use for example.

@amejia1
Member
amejia1 commented Apr 18, 2012

Ok, this now overrides Parse as well and now there's no need for that reference file for the test case. Please review.

@amejia1
Member
amejia1 commented Apr 19, 2012

I took the liberty of cleaning up the commits. The changes here now cover all cases of overriding LoadFile to include the problematic case reported by the Linaro devs.

Please review.

@davilla
davilla commented May 3, 2012

I punt to jm on this, he knows it best.

@wsoltys
Member
wsoltys commented May 3, 2012

the filter file is still broken. A fixed version is here: wsoltys@9184e99

What else did you change since the last working version as it doesn't compile anymore:

1>f:\coding\windows\xbmc_wsoltys\xbmc\utils\xbmctinyxml.cpp(28): error C2039: '{ctor}' : is not a member of 'TiXmlDocument'
1>          f:\coding\windows\xbmc_wsoltys\project\builddependencies\include\tinyxml.h(1393) : see declaration of 'TiXmlDocument'
1>f:\coding\windows\xbmc_wsoltys\xbmc\utils\xbmctinyxml.cpp(33): error C2039: '{ctor}' : is not a member of 'TiXmlDocument'
1>          f:\coding\windows\xbmc_wsoltys\project\builddependencies\include\tinyxml.h(1393) : see declaration of 'TiXmlDocument'
1>f:\coding\windows\xbmc_wsoltys\xbmc\utils\xbmctinyxml.cpp(39): error C2039: '{ctor}' : is not a member of 'TiXmlDocument'
1>          f:\coding\windows\xbmc_wsoltys\project\builddependencies\include\tinyxml.h(1393) : see declaration of 'TiXmlDocument'
1>f:\coding\windows\xbmc_wsoltys\xbmc\utils\xbmctinyxml.cpp(100): warning C4800: 'const char *' : forcing value to bool 'true' or 'false' (performance warning)

I don't have the time to look further atm.

@jmarshallnz
Member

In addition to the commit of @wsoltys, grab this one: jmarshallnz@24f3bd8

win32 then builds and passes the test on Parse(). Also, LoadFile() is correctly called, so win32 looks all good.

@jmarshallnz
Member

@davilla: looks like some osx work is needed to get build sorted (tinyxml dir dropped from project file, dylib's added etc.) I don't see why it wouldn't work once that is done though.

@davilla
davilla commented May 4, 2012

no problem, tinyxml-2.6.2_2 has been building in osx/ios depends for some time now. Just have to fixup the xcode projects projects and we can do that on inject.

@jmarshallnz
Member

In that case, once the 2 fixup commits are rebased in it's ready to go.

@amejia1 amejia1 was assigned May 4, 2012
@amejia1 amejia1 merged commit 4226619 into xbmc:master May 4, 2012
@huceke
Collaborator

This check is wrong. Tinyxml doesn't contain a pkg-config file. Debian added one to their package ( debian/tinyxml.pc ). Other distributions might not include one ( for example Archlinux ).

Member

i confirm, no pkg-config for FreeBSD too
you can check tinyxm.h header or libtinyxml.so lib

Collaborator

Made a PR for it : #927

Also in Slackware I get an error that TINYXML was not found, but I have tinyxml 2.6.2 installed

@FernetMenta

shouldn't this be: data.append(buf, result)?
I get random parse errors, this might be the reason.

Member

yup, and that reserve call that is rather pointless. append will do identically internally. If you'd figure out file size first and reserve with that it would have a point.

Member

From the tinyxml sources

FILE* file = fopen( value.c_str (), "rb" );

   if ( file )
   {
      // Get the file size, so we can pre-allocate the string. HUGE speed impact.
      long length = 0;
      ...
      data.reserve( length );
      ...

So +1 to what elupus said

Member

@vdrfan See ^^, it might explain some of the regression you've seen

Member

@vdrfan note also that upstream reads in 1 big chunk, rather than looping with a smaller buffer. See here.

Member

Yea, unfortunately i had to time to further investigate it.

Member

@vdrfan i've done up a quick patch to nuke the streaming (as you wanted). If you have a min to give it a go on rpi i'd be interested to see the before/after: theuni@44f2703

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