Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

[jpegio] Use chunked reading if CFile::Open doesn't set CFile::GetFileSize #1885

Merged
merged 1 commit into from

2 participants

@jmarshallnz
Owner

This solves the problem mentioned here:

http://forum.xbmc.org/showthread.php?tid=145656

in that some webservers don't set the filesize, or the file isn't seekable (with unknown filesize).

This fixes it by reading chunked.

Alternatives might be to have a general routine available to do this stuff such as CFile::ReadFile(std::vector<uint8_t> &foo) or some such.

Could also be done a little clearer perhaps by separating the chunked and non-chunked reads (the non-chunked read is a single Read() call like it was before).

@elupus, @theuni to review.

@elupus
Collaborator
@jmarshallnz
Owner

Considering we start with either the whole file (most cases) or at least 65k, it's not too bad really considering the size of most jpegs we load (around 200k or so). I don't mind having a doubling type routine but really the time spent in the code here isn't constrained by the slowness of resizing of the buffer ;)

Agreed regarding READ_TRUNCATED and checking the jpeg header, but the latter is a separate issue (also, this code really only gets run on images named "foo.jpg", "foo.tbn" or with mimetype set to "image/jpeg" which somewhat reduces the scope for non-jpeg files being parsed).

@jmarshallnz jmarshallnz merged commit c04aa30 into xbmc:master
@jmarshallnz
Owner

@elupus: pulled into master with changes as suggested by you. Please sign-off for Frodo inclusion.

@elupus
Collaborator
@jmarshallnz
Owner

The stop on read == total is required to ensure the buffer doesn't grow unnecessarily in the case where filesize is known (as amount == 0 at that point).

We also only increase the buffer size if amount == 0, which happens only once we've read in enough to get to the end of the current buffer.

@jmarshallnz jmarshallnz deleted the jmarshallnz:chunky_jpegs branch
@elupus
Collaborator
@jmarshallnz
Owner
  1. Yes. I dunno if this is actually a real issue or not. If it is, it would probably be best to initially make chunksize slightly larger (one byte is enough) than the actual filesize, so that the second Read() operation can be done in the case where GetLength() is accurate (the second Read() returning 0 and breaking the loop). If chunksize == file length, then there's always a realloc() otherwise.

  2. Nope. We resize only when we've completely filled the existing buffer (amount == 0), which happens only once we've read chunksize bytes. We'd allocate 65k in that case.

@jmarshallnz
Owner

The code is be a bit clearer after 9e7d52f and also takes care of the expanding file case (0 < GetLength() < real file size)

@elupus
Collaborator
@jmarshallnz
Owner

Heh - it is kinda unobvious exactly how all the cases drop through, and is nicer without that check for total == filesize. I figured this code will end up as some CFileUtils::ReadFile() or some such (or even CFile I guess?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 8, 2013
  1. [jpegio] use chunked reading in case the filesize isn't available on …

    Jonathan Marshall authored
    …Open()
This page is out of date. Refresh to see the latest.
Showing with 35 additions and 7 deletions.
  1. +35 −7 xbmc/guilib/JpegIO.cpp
View
42 xbmc/guilib/JpegIO.cpp
@@ -246,23 +246,51 @@ CJpegIO::~CJpegIO()
void CJpegIO::Close()
{
- delete [] m_inputBuff;
+ free(m_inputBuff);
+ m_inputBuffSize = 0;
}
bool CJpegIO::Open(const CStdString &texturePath, unsigned int minx, unsigned int miny, bool read)
{
+ Close();
+
m_texturePath = texturePath;
- unsigned int imgsize = 0;
XFILE::CFile file;
- if (file.Open(m_texturePath.c_str(), 0))
+ if (file.Open(m_texturePath.c_str(), READ_TRUNCATED))
{
- imgsize = (unsigned int)file.GetLength();
- m_inputBuff = new unsigned char[imgsize];
- m_inputBuffSize = file.Read(m_inputBuff, imgsize);
+ unsigned int filesize = (unsigned int)file.GetLength();
+ unsigned int chunksize = filesize, maxchunksize = filesize;
+ if (!chunksize)
+ { // no size information, so try reading chunked
+ chunksize = std::max(65536U, (unsigned int)file.GetChunkSize());
+ maxchunksize = std::max(chunksize, 2048*1024U); // max 2MB
+ }
+ unsigned int total = 0, amount = 0;
+ while (true)
+ {
+ if (!amount)
+ { // (re)alloc
+ m_inputBuffSize += chunksize;
+ m_inputBuff = (unsigned char *)realloc(m_inputBuff, m_inputBuffSize);
+ if (!m_inputBuff)
+ {
+ CLog::Log(LOGERROR, "%s unable to allocate buffer of size %u", __FUNCTION__, m_inputBuffSize);
+ return false;
+ }
+ amount = chunksize;
+ chunksize = std::min(chunksize*2, maxchunksize);
+ }
+ unsigned int read = file.Read(m_inputBuff + total, amount);
+ amount -= read;
+ total += read;
+ if (!read || total == filesize)
+ break;
+ }
+ m_inputBuffSize = total;
file.Close();
- if ((imgsize != m_inputBuffSize) || (m_inputBuffSize == 0))
+ if (m_inputBuffSize == 0)
return false;
}
else
Something went wrong with that request. Please try again.