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

gsdx: dump gsdump in xz format directly #1922

Merged
merged 2 commits into from May 21, 2017

Conversation

Projects
None yet
3 participants
@gregory38
Contributor

gregory38 commented Apr 30, 2017

Reduce disk space. Easy to share.

@turtleli @FlatOutPS2
It would be nice to port this code to Windows. (and the readback too)
libzma code was taken from https://git.tukaani.org/?p=xz.git

@turtleli

This comment has been minimized.

Member

turtleli commented May 6, 2017

Hmm. Wouldn't it takes ages to compress long multi-frame GS dumps?

Personally I think it's fine to just compress afterwards in xz format so multiple threads can be used and compression can be maximised. I think the GS dump instructions could be updated so it mentions compressing the dump in xz format - I don't think most users actually know that that's what is preferred.

@gregory38

This comment has been minimized.

Contributor

gregory38 commented May 6, 2017

Well I didn't tested the speed on long dump. Most of the dumps are short. At least I'm in favour to automatically compress the short dump.

I'm not sure that MT is a good idea. It will badly impact the compression ratio.

I will factorize the code to keep both compressed and not compressed

@turtleli

This comment has been minimized.

Member

turtleli commented May 6, 2017

Well I didn't tested the speed on long dump. Most of the dumps are short. At least I'm in favour to automatically compress the short dump.

Well I think the main issue with compressing a long dump with the current implementation is if the size passes 200MB - emulation will freeze and the user will have to keep holding shift for a while waiting for it to unfreeze if they still need more frames for whatever reason.

I'm not sure that MT is a good idea. It will badly impact the compression ratio.

Oh you're right, it does seem to affect it quite a bit.

@gregory38

This comment has been minimized.

Contributor

gregory38 commented May 6, 2017

Yes I know. Maybe we can push it further. 500MB/1000MB. I don't think we need more. Besides it would just crash when we read back the dump.(well I think we can use a 64 bits gsdx now)

But honestly I would be fine to keep compression for only short dump.

Data on the dump is quite repetitive. So yes having a single dictionary is way better. Hum I never tested but maybe ratio can remain high enough with lower compression level.

@gregory38

This comment has been minimized.

Contributor

gregory38 commented May 7, 2017

In order to have some numbers, I did a quick test on my biggest dump (1.2 GB).

xz -4 -k GT3_R11_Slowdown_2.gs --stdout > gt.4.xz  172.06s user 0.32s system 99% cpu 2:52.38 total
275M May  7 13:20 gt.4.xz
xz -5 -k GT3_R11_Slowdown_2.gs --stdout > gt.5.xz  217.20s user 0.29s system 99% cpu 3:37.49 total
113M May  7 13:38 gt.5.xz
xz -6 -k GT3_R11_Slowdown_2.gs --stdout > gt.6.xz  290.68s user 0.28s system 99% cpu 4:50.97 total
110M May  7 13:27 gt.6.xz

-7/-8/-9 are slower for no gain.

I propose that we keep the compressed version for the short dump to ease sharing of them.

Edit: I might need to remove the now 1GB limit of the code.

@gregory38

This comment has been minimized.

Contributor

gregory38 commented May 9, 2017

@turtleli
Any opinion on the interface ? Is is better to use inheritance or add a boolean check ? (or other ?)

@turtleli

This comment has been minimized.

Member

turtleli commented May 10, 2017

In this case I'd prefer inheritance, I think it'd be more readable.

The interface - I'm not keen on some parts. Personally I'd do this for the base class:

class GSDumpBase
{
public:
	GSDumpBase();
	virtual ~GSDumpBase() = default;

	void ReadFIFO(uint32 size);
	void Transfer(int index, const uint8* mem, size_t size);
	bool VSync(int field, bool last, const GSPrivRegSet* regs);

protected:
	void AddDumpHeader(uint32 crc, const GSFreezeData& fd, const GSPrivRegSet* regs);

private:
	virtual void AppendRawData(const void *data, size_t size) = 0;
	virtual void AppendRawData(uint8 c) = 0;

	int m_frames;
	int m_extra_frames;
};

so opening, writing, and closing the file is entirely up to the derived classes,, but everything else is taken care of by the base class. Well, opening and closing could perhaps be done by the base class too. For the derived classes, I'd do something like this:

class GSDump final: public GSDumpBase
{
public:
	GSDump(const std::string& fn, uint32 crc, const GSFreezeData& fd, const GSPrivRegSet* regs);
	~GSDump() override;

private:
	void AppendRawData(const void *data, size_t size) override;
	void AppendRawData(uint8 c) override;

	FILE* m_gs;
};

class GSDumpXz final: public GSDumpBase
{
public:
	GSDumpXz(const std::string& fn, uint32 crc, const GSFreezeData& fd, const GSPrivRegSet* regs);
	~GSDumpXz() override;

private:
	void AppendRawData(const void *data, size_t size) override;
	void AppendRawData(uint8 c) override;

	void Flush(bool close);

	FILE* m_gs;
	lzma_stream m_strm;
	std::vector<uint8> m_in_buff;
};

That way, long dumps can be written straight to file without devouring memory, and short dumps can be buffered, xz compressed, and then written to file.

@gregory38 gregory38 force-pushed the greg/gsdx-lzma-dump branch from c380db3 to 9ae05d2 May 10, 2017

@gregory38

This comment has been minimized.

Contributor

gregory38 commented May 11, 2017

@turtleli is this way better ? Note: I need to test the code.

public:
GSDump();
virtual ~GSDump();
GSDumpBase(const string& fn);

This comment has been minimized.

@turtleli

turtleli May 11, 2017

Member

std::

Ditto for all other constructors.

{
m_gs = fopen(fn.c_str(), "wb");

This comment has been minimized.

@turtleli

turtleli May 11, 2017

Member

This could be part of the member initialiser list too (though it's up to you).

m_in_buff.push_back(c);
}

This comment has been minimized.

@turtleli

turtleli May 11, 2017

Member

Single empty line instead of two?

GSDumpXz::GSDumpXz(const string& fn, uint32 crc, const GSFreezeData& fd, const GSPrivRegSet* regs) : GSDumpBase(fn + ".gs.xz")
{
m_in_buff.clear();

This comment has been minimized.

@turtleli

turtleli May 11, 2017

Member

This line is unnecessary - the vector will already be empty.

Also, I think m_gs needs to be checked, otherwise there might be a memleak (lzma_end() won't be called I think).

m_dump = std::unique_ptr<GSDumpBase>(new GSDumpXz(m_snapshot, m_crc, fd, m_regs));
#else
m_dump = std::unique_ptr<GSDumpBase>(new GSDump(m_snapshot, m_crc, fd, m_regs));
#endif

This comment has been minimized.

@turtleli

turtleli May 11, 2017

Member

Perhaps a function should be added to check that everything initialised correctly. If something fails, dump writing can be aborted immediately.

This comment has been minimized.

@gregory38

gregory38 May 11, 2017

Contributor

I don't think it worth the extra code. However I did an early abort on Vsync. It should be enough.

This comment has been minimized.

@turtleli

turtleli May 12, 2017

Member

Is Vsync the first thing that is added to the dump after the header?

I think you might need to add an if (m_gs == nullptr) check in the GSDump constructor to prevent some error message spam if the file can't be opened.

This comment has been minimized.

@gregory38

gregory38 May 12, 2017

Contributor

First Vsync is the next frame, so yes could be verbose. We need to do an initial Vsync when replaying.

I created a Write function to check size and m_gs properly. It should be better now. Is it ok for you ?

This comment has been minimized.

@turtleli

turtleli May 12, 2017

Member

Well, personally I would have added an init check function (there's less thinking required about what would happen if things go wrong), but yeah, it looks better.

// Enough data was accumulated, time to write/compress it. If compression
// is enabled, it will freeze PCSX2. 1GB should be enough for long dump.
if (m_in_buff.size() > 1024*1024*1024)
Flush(false);

This comment has been minimized.

@turtleli

turtleli May 11, 2017

Member

I guess the if won't ever be true anymore.

Actually, unless I'm misreading something, I think there's a small risk that lzma_code(&m_strm, LZMA_FINISH) won't be called if this path is ever taken (VSync -> flush(false) -> empty buffer -> destructor -> flush(true) -> early return -> lzma_code(&m_strm, LZMA_FINISH) not called + memleak because lzma_end() isn't called).

This comment has been minimized.

@gregory38

gregory38 May 11, 2017

Contributor

With the smart_ptr, there is no reason to close the stream in the flush. So I will move it in the destructor.

However, I'm not sure of what will happen if lzma_code(&m_strm, LZMA_FINISH) isn't called. Neither if I always call it in the destructor with a zero sized buffer.

This comment has been minimized.

@gregory38

gregory38 May 11, 2017

Contributor

actually it seems to crash :(
edit: might just need an output buffer.

This comment has been minimized.

@gregory38

gregory38 May 11, 2017

Contributor

Yeah I missed with an output buffer to contain the end of stream. I tested short and long dump and it works as expected now.

// GSDump implementation
//////////////////////////////////////////////////////////////////////
GSDump::GSDump(const string& fn, uint32 crc, const GSFreezeData& fd, const GSPrivRegSet* regs) : GSDumpBase(fn + ".gs")

This comment has been minimized.

@turtleli

turtleli May 11, 2017

Member

Maybe put the base class constructor on a new line? IMO it's a bit hard to notice. Ditto GSDumpXz.

fputc(2, m_gs);
fwrite(&size, 4, 1, m_gs);
m_strm = LZMA_STREAM_INIT;
lzma_ret ret = lzma_easy_encoder(&m_strm, 6 /*level*/, LZMA_CHECK_CRC64);

This comment has been minimized.

@turtleli

turtleli May 12, 2017

Member

Hmm. In the unlikely (I think?) case that it fails, it'll leave an empty file behind.

void GSDumpXz::Compress(lzma_action action, lzma_ret expected_status)
{
std::vector<uint8> out_buff(1024*1024);

This comment has been minimized.

@turtleli

turtleli May 12, 2017

Member

Would it be better to use an std::array here?

This comment has been minimized.

@gregory38

gregory38 May 13, 2017

Contributor

yes, I will change it with the .data()

{
std::vector<uint8> out_buff(1024*1024);
do {
m_strm.next_out = &out_buff[0];

This comment has been minimized.

@turtleli

turtleli May 12, 2017

Member

As an alternative, out_buff.data() could be used instead.

@gregory38 gregory38 force-pushed the greg/gsdx-lzma-dump branch from f47d4ca to 7cb97ad May 13, 2017

@gregory38

This comment has been minimized.

Contributor

gregory38 commented May 16, 2017

@turtleli is it good for you ?

@turtleli

This comment has been minimized.

Member

turtleli commented May 16, 2017

Well, it looks ok, though the empty file creation if the xz init fails isn't ideal.

if (last)
m_extra_frames--;
return ((++m_frames & 1) == 0 && last && (m_extra_frames < 0));

This comment has been minimized.

@turtleli

turtleli May 17, 2017

Member

Some unnecessary parentheses have been used.

FILE* m_gs;
protected:

This comment has been minimized.

@turtleli

turtleli May 17, 2017

Member

Unnecessary blank line.

m_dump.VSync(field, !control, m_regs);
}
if (m_dump->VSync(field, !m_control_key, m_regs))

This comment has been minimized.

@turtleli

turtleli May 17, 2017

Member

Would be nice to clang-format GSdx and PCSX2 some day. ;)

This comment has been minimized.

@gregory38

gregory38 May 17, 2017

Contributor

It is the first thing that I will do after the release. And I will likely do the core too.

This comment has been minimized.

@gregory38

gregory38 May 19, 2017

Contributor

Update done. Thanks for the review.

gsdx: dump gsdump in xz format directly
Reduce disk space. Easy to share.

It would be nice to port the code to Windows.
libzma code was taken from https://git.tukaani.org/xz.git

Note: only short dumps are supported so far. Big dump will freeze the interface during the compression.
Or will suck all the RAM.
Note2: a multithreaded encoder would badly impact the compression ratio

Thanks to Turtleli for all review comments

@gregory38 gregory38 force-pushed the greg/gsdx-lzma-dump branch from 7cb97ad to 5c97621 May 19, 2017

@gregory38

This comment has been minimized.

Contributor

gregory38 commented May 21, 2017

@turtleli Could the code be merged ? Could you take care of the Win side :)

@turtleli

This comment has been minimized.

Member

turtleli commented May 21, 2017

Yeah, you can merge it.

As for Windows side, it might take a while - I might experiment with submodules a bit.

@gregory38

This comment has been minimized.

Contributor

gregory38 commented May 21, 2017

How long is a "while" for you ? Weeks/Months ?

@turtleli

This comment has been minimized.

Member

turtleli commented May 21, 2017

Maybe a week or two? It's mostly to figure out how best to handle VS project files when building stuff in submodules.

@gregory38

This comment has been minimized.

Contributor

gregory38 commented May 21, 2017

No pressure, it is fine ;) I'm looking forward for the submodule support.

@gregory38 gregory38 merged commit 802f102 into master May 21, 2017

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@gregory38 gregory38 deleted the greg/gsdx-lzma-dump branch May 23, 2017

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