Add Linux Port #9

BusyStudent · 2021-09-22T14:07:42Z

Hi,
I have made a linux port for it,It can be compiled
But when I run the test,the tga file has be exported
the psd file could not be exoported,

***ERROR*** [ImageResources] Image resources section seems to be corrupt, signature does not match "8BIM".
***ERROR*** [ImageResources] Image resources section seems to be corrupt, signature does not match "8BIM".

MolecularMatters · 2021-09-22T15:32:39Z

Hi,

Did you get #8?
That should fix the issue you're seeing.

BusyStudent · 2021-09-22T16:04:45Z

I have #8,but it doesnot fix the issue,
I had fix the error in writing psd,now the psd can be exported in the test

MolecularMatters · 2021-09-22T16:13:40Z

If I understand you correctly, writing PSDs now works, but reading the image resources section is still broken?

BusyStudent · 2021-09-22T16:19:29Z

Yes,But not all image resources sections were broken,only two sections are still broken

BusyStudent · 2021-09-23T14:06:25Z

After debugging
I found that the position read in the function ParseImageResourcesSection was wrong when the error occurred.
He points to 16635 but the correct position is 16636

And this section type is WINDOWS_DEVMODE

const uint32_t signature = fileUtil::ReadFromFileBE<uint32_t>(reader);
if ((signature != util::Key<'8', 'B', 'I', 'M'>::VALUE) && (signature != util::Key<'p', 's', 'd', 'M'>::VALUE))
{
       //reader.m_postiton = 16635 before read
       //But the right postiton is 16636

	PSD_ERROR("ImageResources", "Image resources section seems to be corrupt, signature does not match \"8BIM\".");
	return imageResources;
}

I don’t think the issue come from I/O
Because when I use synchronous I/O this problem still occurred

Reading and writing psd now works normally,except for this secetion

Sorry for my poor english

MolecularMatters · 2021-09-23T15:31:25Z

One byte off sounds like a round-up-to-2/4/8 is missing somewhere.
Looking at ParseImageResourcesSection, the position to read the image resources section comes directly from document->imageResourcesSection.offset, so that value is wrong in this case.

If you look at PsdParseDocument.cpp, the offsets into different sections are all grabbed when creating the document in Create(). I'm not sure if the code has to round up those offsets to the next even value, I'll have to check the PSD spec on that.

One question though: does this happen with the default PSD file that ships with the sample code?
If it does, I would be surprised that this works on Windows, but not on Linux, because the file is the same, which means that the offset has to be the same on both platforms.

BusyStudent · 2021-09-23T15:48:28Z

It does

MolecularMatters · 2021-09-23T15:59:47Z

Ah wait, this is not an off-by-one error.
When reading the offsets into the sections, there is no need to round up or anything. The code fetches the offset from the reader directly using reader.GetPosition().
The correct offset for the image resources section is 34. This is where you'll also find the 8BIM marker in the PSD file.

If your code is pointing at offset 16635, I would think GetPosition() does not work correctly on Linux yet.

BusyStudent · 2021-09-23T16:33:43Z

Emm,SyncFileReader::GetPosition just return m_postion

// ---------------------------------------------------------------------------------------------------------------------
// ---------------------------------------------------------------------------------------------------------------------
void SyncFileReader::Read(void* buffer, uint32_t count)
{
	// do an asynchronous read, wait until it's finished, and update the file position
	File::ReadOperation op = m_file->Read(buffer, count, m_position);
	m_file->WaitForRead(op);

	m_position += count;
}


// ---------------------------------------------------------------------------------------------------------------------
// ---------------------------------------------------------------------------------------------------------------------
void SyncFileReader::Skip(uint64_t count)
{
	m_position += count;
}


// ---------------------------------------------------------------------------------------------------------------------
// ---------------------------------------------------------------------------------------------------------------------
void SyncFileReader::SetPosition(uint64_t position)
{
	m_position = position;
}


// ---------------------------------------------------------------------------------------------------------------------
// ---------------------------------------------------------------------------------------------------------------------
uint64_t SyncFileReader::GetPosition(void) const
{
	return m_position;
}

I donnot know which part was wrong

Reading the section at offset 34 is succeed,
But Reading the section at offset 16635 is failed,the 8BIM marker is on 16636,
Although it is not an important section,it will be skiped bacause the type is WINDOWS_DEVMODE

MolecularMatters · 2021-09-23T16:42:36Z

Apologies, I just noticed that the Windows code also reads from offset 16635, so there's still a bug in there somewhere.
I'll fix this first, so you can try your commits again afterwards.

MolecularMatters · 2021-09-24T07:32:47Z

I just fixed this in 91643c3, can you please give this another try?
If your code now works on Linux, I'll review it today.

BusyStudent · 2021-09-24T09:15:59Z

OK,It works on Linux

BusyStudent · 2021-09-25T14:03:17Z

@MolecularMatters Did you complete the code review?It now works

src/Psd/PsdNativeFile_Linux.cpp

src/Psd/PsdNativeFile_Linux.h

MolecularMatters · 2021-09-28T09:14:00Z

src/Samples/PsdSamples.cpp

+ #ifdef _WIN32
+ //In Windows wchar_t is utf16
+ static_assert(sizeof(wchar_t) == sizeof(uint16_t));
 layerName << reinterpret_cast<wchar_t*>(layer->utf16Name);
+ #else
+ //In Linux, wchar_t is utf32
+ //Convert code from https://stackoverflow.com/questions/23919515/how-to-convert-from-utf-16-to-utf-32-on-linux-with-std-library#comment95663809_23920015
+ //We cannot use C++11,so use macro instead of lambda
+ #define is_surrogate(uc) ( (uc - 0xd800u) < 2048u )
+ #define is_high_surrogate(uc) ( (uc & 0xfffffc00) == 0xd800 )
+ #define is_low_surrogate(uc) ( (uc & 0xfffffc00) == 0xdc00 )
+ #define surrogate_to_utf32(high,low) ((high << 10) + low - 0x35fdc00)
+ static_assert(sizeof(wchar_t) == sizeof(uint32_t));
+
+ //Begin convert
+ size_t u16len = 0;
+ const uint16_t * cur = layer->utf16Name;
+ while(*cur != uint16_t('\0')){
+ cur++;
+ u16len++;
+ }
+ //Len it
+
+ const uint16_t * const end = layer->utf16Name + u16len;
+ const uint16_t * input = layer->utf16Name;
+ while (input < end) {
+ const uint16_t uc = *input++;
+ if (!is_surrogate(uc)) {
+ layerName << wchar_t(uc); 
+ } 
+ else {
+ if (is_high_surrogate(uc) && input < end && is_low_surrogate(*input)){
+ layerName << wchar_t(surrogate_to_utf32(uc, *input++));
+ }
+ else{
+ // Error
+ // Impossible
+ std::abort();
+ }
+ }
+ }
+ #undef is_surrogate
+ #undef is_high_surrogate
+ #undef is_low_surrogate
+ #undef surrogate_to_utf32
+ #endif


I think it would be fine to use C++11 in this part of the sample code, which would probably simplify it a lot.
We could also make it so that when using a pre-C++11 compiler under Linux, the layername simply won't be output, so that the code doesn't require C++11, but rather uses it if available.

I could not find the way to convert utf16 to utf32 with std library

src/Samples/PsdTgaExporter.cpp

MolecularMatters · 2021-09-28T09:16:09Z

src/Samples/PsdTgaExporter.cpp

+#include <cwchar>
+#include <cstring>
+#include <cstdlib>
+#define OutputDebugStringA(S) fputs(S,stderr)


I think it would be better to also put this into a separate file.
Maybe get rid of OutputDebugString altogether and change it into something platform-agnostic, which then calls OutputDebugStringA on Windows, and fputs on Linux?

I had put them into PsdDebug.h

MolecularMatters

Thank you for your effort so far!
I think the code can be improved by paying a bit more attention to details, see my review comments.

BusyStudent · 2021-10-02T05:19:11Z

Ok,I had finished it

BusyStudent · 2021-10-09T11:55:58Z

Did any where need to be changed?

MolecularMatters · 2021-10-18T12:03:51Z

Apologies, this fell off the radar last week.

Add Linux Port

3cbd4ab

fix write error

a41bac3

BusyStudent and others added 2 commits September 24, 2021 17:09

Merge branch 'MolecularMatters:master' into master

7f02fc4

Fix invalid read

629f0e6