RetroPlayer: Merge savestate metadata into .sav file #14209

garbear · 2018-07-21T01:06:57Z

Description

Introduced in #12434, RetroPlayer uses two files for autosave: a .sav file for the memory data, and an .xml file for metadata. This combines both memory data and metadata into a single .sav file:

Chrono Trigger (U) [!].smc   (the ROM)
Chrono Trigger (U) [!].sav   (the state and metadata)

In this PR, FlatBuffers is used to serialize metadata. This library was chosen for performance reasons, as it is the only popular serialization library that allows for zero-copy. Although it requires a native compiler, the target library is headers-only.

Backwards compatibility with XML files was not retained due to the extra code it would take, and RetroPlayer is still alpha software.

As a result of this PR, savestates are several hundred kb smaller, half as many files, and loading/saving should be faster on slow devices due to skipping costly XML operations.

I spent a LONG time on the build system stuff, and I'm not sure if I did everything correctly.

Motivation and Context

First abstraction for the new Saved Games manager.

How Has This Been Tested?

Tested on Windows 7 and OSX.

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)
Cosmetic change (non-breaking change that doesn't touch code)
None of the above (please explain below)

garbear · 2018-07-22T20:12:54Z

TODO: The savestate needs a version field

(testing depends) jenkins build this please

garbear · 2018-07-22T22:17:16Z

jenkins build this please

garbear · 2018-07-24T05:03:42Z

jenkins build this please

xbmc/cores/RetroPlayer/messages/CMakeLists.txt

+)
+
+core_add_messages(retroplayer_messages)
+set(EXTRA_INCLUDES ${EXTRA_INCLUDES} PARENT_SCOPE)


wsnipex

cmake stuff doesn't conform to our standards and needs serious changes.

move all flatbuffer building to depends
fix the findmodule to allow bulding with external and internal flatbuffer and just use FLATBUFFER_INCLUDE_DIR
no changes to core_add_library

finally: flatbuffers is not available for several linux distros, incl ubuntu

Rechi

flatc and flatbuffers has to be added to https://github.com/xbmc/kodi-deps/, so it can be easily build from source (for windows) if needed.

garbear · 2018-07-26T07:53:38Z

move all flatbuffer building to depends

I think this is the case. BuildFlatBuffers.cmake is not used.

fix the findmodule to allow bulding with external and internal flatbuffer and just use FLATBUFFER_INCLUDE_DIR

Is there an existing example I can learn from?

no changes to core_add_library

How do I make other code depend on the generated headers?

finally: flatbuffers is not available for several linux distros, incl ubuntu

Can we build flatc when not detected, à la TexturePacker?

Rechi · 2018-07-26T07:56:24Z

FFmpeg, Crossguid, Fmt, fstrcmp and RapidJSON can be built internal.

cmake/modules/FindFlatBuffers.cmake

+  set(FLATBUFFERS_INCLUDE_DIRS ${FLATBUFFERS_INCLUDE_DIR})
+  include_directories(${CMAKE_BINARY_DIR})
+else()
+  set(FLATBUFFERS_INCLUDE_DIR)


cmake/modules/FindFlatBuffers.cmake

+  set(FLATBUFFERS_INCLUDE_DIR)
+endif()
+
+include("${FLATBUFFERS_CMAKE_DIR}/BuildFlatBuffers.cmake")


project/BuildDependencies/scripts/0_package.native-win32.list

@@ -11,3 +11,4 @@ jsonschemabuilder-1.0.0-win32-3.7z
 swig-3.0.10-win32.7z
 texturepacker-1.1.1-win32.7z
 vswhere-2.2.11-win32.7z
+flatc-1.9.0-win32.7z


tools/depends/native/flatbuffers-native/Makefile

+VERSION=1.9.0
+SOURCE=v$(VERSION)
+ARCHIVE=$(SOURCE).tar.gz
+BASE_URL=https://github.com/google/flatbuffers/archive


tools/depends/target/flatbuffers/Makefile

+	ROOT_DIR := $(shell dirname $(realpath $(lastword $(MAKEFILE_LIST))))
+	PLATFORM = native
+	TARBALLS_LOCATION = $(ROOT_DIR)
+	BASE_URL := https://github.com/google/flatbuffers/archive


xbmc/cores/RetroPlayer/savestates/SavestateFlatBuffer.h

+
+#include "ISavestate.h"
+
+#include "flatbuffers/flatbuffers.h"


cmake/modules/FindFlatBuffers.cmake


 include(FindPackageHandleStandardArgs)
 find_package_handle_standard_args(flatbuffers
  DEFAULT_MSG FLATBUFFERS_FLATC_EXECUTABLE FLATBUFFERS_INCLUDE_DIR)

-if(FLATBUFFERS_FOUND)
  function(FLATBUFFERS_GENERATE_C_HEADERS Name)


wsnipex · 2018-07-30T09:52:46Z

@garbear see wsnipex@67e6b8d for some cmake fixes.
Not yet perfect at all, but work for building with internal flatbuffers on linux(without depends)

cmake/scripts/common/Macros.cmake

@@ -813,3 +813,4 @@ macro(find_addon_xml_in_files)
  # Append also versions.h to depends
  list(APPEND ADDON_XML_DEPENDS "${CORE_SOURCE_DIR}/xbmc/addons/kodi-addon-dev-kit/include/kodi/versions.h")
 endmacro()
+


tools/depends/target/flatbuffers/Makefile

+                 -DFLATBUFFERS_BUILD_TESTS=OFF \
+                 -DFLATBUFFERS_INSTALL=ON \
+                 -DFLATBUFFERS_BUILD_FLATLIB=OFF \
+                 -DFLATBUFFERS_BUILD_FLATC=$(BUILD_FLATC) \


wsnipex · 2018-07-31T07:00:54Z

small fix for external flatbuffers: wsnipex@46527b8

garbear · 2018-08-08T20:27:55Z

jenkins build this please

garbear · 2018-08-08T20:39:57Z

jenkins build this please

xbmc/cores/RetroPlayer/messages/CMakeLists.txt

+set_target_properties(retroplayer_messages PROPERTIES FOLDER "Generated Messages"
+                                                      INCLUDE_DIRECTORIES ${CMAKE_CURRENT_BINARY_DIR}
+                                                      SOURCES ${FLATC_OUTPUTS})
+add_dependencies(retroplayer_messages flatbuffers)


wsnipex

only one minor, otherwise looks good to me

cmake/modules/FindFlatBuffers.cmake

+else()
+  find_program(FLATBUFFERS_FLATC_EXECUTABLE NAMES flatc)
+  find_path(FLATBUFFERS_INCLUDE_DIR NAMES flatbuffers/flatbuffers.h)
+  set(FLATBUFFERS_MESSAGES_INCLUDE_DIR ${CMAKE_BINARY_DIR}/${CORE_BUILD_DIR}/cores/RetroPlayer/messages CACHE INTERNAL "Generated Flatbuffer headers")


garbear · 2018-08-11T19:04:23Z

@wsnipex updated. jenkins build this please

garbear · 2018-08-11T19:40:27Z

jenkins build this please

garbear · 2018-08-11T19:51:40Z

jenkins build this please

garbear · 2018-08-11T19:57:53Z

jenkins build this please

garbear · 2018-08-11T21:01:18Z

all green

garbear · 2018-08-11T22:34:00Z

jenkins build this please

The code was originally placed in games/ to minimize the size of RetroPlayer, which was crucial to rapidly scaling the player to handle the rendering system added in the GSoC 2017 shader project. Code shuffle only. No functional changes.

This transfers ownership of the playback system from the game client to RetroPlayer. Our goal is to move the memory stream (currently owned by playback) into a RetroPlayerMemory stream.

Code shuffle only. No functional changes.

Rechi · 2018-08-14T07:01:35Z

Fedora has a flatbuffers package, so documentation has to be adjusted.

This adds the flatc compiler to native depends and the flatbuffer headers to target depends.

This removes the auxiliary XML metadata file used for savestates. Now, metadata is serialized into the .sav file using FlatBuffers.

garbear · 2018-08-14T18:30:06Z

added package to fedora. jenkins build this please

garbear · 2018-08-14T23:19:06Z

thanks @Rechi @wsnipex for all your help getting this in!

[cleanup] remove retroplayer leftovers after #14209

garbear added v18 Leia Component: Games labels Jul 21, 2018

garbear added this to the Leia 18.0-alpha3 milestone Jul 21, 2018

garbear added Component: Depends CMake labels Jul 21, 2018

garbear force-pushed the savestate-metadata branch from a4099a3 to 01ea551 Compare July 21, 2018 01:19

garbear force-pushed the savestate-metadata branch 2 times, most recently from 363b707 to 8072f5d Compare July 22, 2018 23:53

garbear commented Jul 24, 2018

View reviewed changes

xbmc/cores/RetroPlayer/messages/CMakeLists.txt Outdated

)

core_add_messages(retroplayer_messages)

set(EXTRA_INCLUDES ${EXTRA_INCLUDES} PARENT_SCOPE)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

wsnipex requested changes Jul 26, 2018

View reviewed changes

Rechi requested changes Jul 26, 2018

View reviewed changes

garbear force-pushed the savestate-metadata branch from 0984934 to ba8ec8e Compare July 27, 2018 06:45

Rechi reviewed Jul 27, 2018

View reviewed changes

jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Jul 27, 2018

garbear force-pushed the savestate-metadata branch from 4129ef2 to d101aaf Compare July 30, 2018 03:56

garbear commented Jul 30, 2018

View reviewed changes

garbear force-pushed the savestate-metadata branch from d101aaf to 4a6076d Compare July 31, 2018 06:05

jenkins4kodi removed the Rebase needed PR that does not apply/merge cleanly to current base branch label Jul 31, 2018

garbear force-pushed the savestate-metadata branch 2 times, most recently from fd0fe81 to ef86085 Compare July 31, 2018 06:10

Rechi reviewed Jul 31, 2018

View reviewed changes

MartijnKaijser removed this from the Leia 18.0-alpha3 milestone Jul 31, 2018

garbear force-pushed the savestate-metadata branch from cd5131b to f5727d9 Compare July 31, 2018 20:42

garbear closed this Aug 8, 2018

garbear reopened this Aug 8, 2018

Rechi reviewed Aug 8, 2018

View reviewed changes

garbear force-pushed the savestate-metadata branch from 58b429a to 742fab4 Compare August 9, 2018 20:40

wsnipex reviewed Aug 10, 2018

View reviewed changes

garbear force-pushed the savestate-metadata branch from 59fa126 to 14b1b66 Compare August 11, 2018 19:56

garbear added 4 commits August 13, 2018 12:03

Fix rebase error in 1655ee9 (PR:14169)

5435aba

RetroPlayer: Move ownership of playback out of game client

e5ba623

This transfers ownership of the playback system from the game client to RetroPlayer. Our goal is to move the memory stream (currently owned by playback) into a RetroPlayerMemory stream.

RetroPlayer: Import savestate code from game add-ons

4abbd40

Code shuffle only. No functional changes.

garbear force-pushed the savestate-metadata branch from 31673d3 to 61a5cc1 Compare August 13, 2018 19:04

wsnipex approved these changes Aug 14, 2018

View reviewed changes

garbear added 2 commits August 14, 2018 11:29

tools/depends: Add FlatBuffers v1.9.0 compiler and headers

cd50c4f

This adds the flatc compiler to native depends and the flatbuffer headers to target depends.

RetroPlayer: Merge savestate metadata into .sav file

3ed099a

This removes the auxiliary XML metadata file used for savestates. Now, metadata is serialized into the .sav file using FlatBuffers.

garbear force-pushed the savestate-metadata branch from 587ecfb to 3ed099a Compare August 14, 2018 18:29

garbear merged commit fc9faf3 into xbmc:master Aug 14, 2018

garbear deleted the savestate-metadata branch August 14, 2018 23:19

Rechi added this to the Leia 18.0-beta1 milestone Aug 16, 2018

Rechi added a commit that referenced this pull request Apr 8, 2019

Merge pull request #15879 from Rechi/cleanup/retroplayer

e43243f

[cleanup] remove retroplayer leftovers after #14209


		#include "ISavestate.h"

		#include "flatbuffers/flatbuffers.h"

RetroPlayer: Merge savestate metadata into .sav file #14209

RetroPlayer: Merge savestate metadata into .sav file #14209

Conversation

garbear commented Jul 21, 2018 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of change

garbear commented Jul 22, 2018

garbear commented Jul 22, 2018

garbear commented Jul 24, 2018

This comment was marked as spam.

This comment was marked as spam.

wsnipex left a comment

Choose a reason for hiding this comment

Rechi left a comment

Choose a reason for hiding this comment

garbear commented Jul 26, 2018 • edited Loading

Rechi commented Jul 26, 2018

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

wsnipex commented Jul 30, 2018

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

wsnipex commented Jul 31, 2018

garbear commented Aug 8, 2018

garbear commented Aug 8, 2018

This comment was marked as spam.

This comment was marked as spam.

wsnipex left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

garbear commented Aug 11, 2018

garbear commented Aug 11, 2018

garbear commented Aug 11, 2018

garbear commented Aug 11, 2018

garbear commented Aug 11, 2018

garbear commented Aug 11, 2018

Rechi commented Aug 14, 2018

garbear commented Aug 14, 2018

garbear commented Aug 14, 2018

garbear commented Jul 21, 2018 •

edited

Loading

garbear commented Jul 26, 2018 •

edited

Loading