Retroarch: Playlist JSON format is not really JSON (sensitive to whitespace)

[geajack]

Description

I went to edit my playlist .lpl files by hand to clear out a bunch of stuff added by Scan Directory which I didn't want in there. I modified them with Python and didn't bother to format the JSON, just removing all whitespace and putting it all on one line.

This is sort of a big deal since currently RetroArch lacks good in-UI facilities for modifying playlists, so sometimes you need to edit them by hand. Aside from that though, rolling your own JSON parser instead of just using a library to parse it properly is generally seen as bad coding practice, and for a good reason... it'll make it harder for you guys to update the playlist format in future, and is just giving bugs more places to hide in the code. Unless there's a very good reason for it I can't think why you wouldn't want to just use some open source C JSON library.

Expected behavior

RetroArch parses .lpl files like JSON, ignoring whitespace appropriately. Playlists with different whitespace formatting is parsed and displayed in the UI correctly.

Actual behavior

RetroArch is sensitive to JSON, including how many spaces are used to indent or whether brackets go on their own line or not. The playlist is not displayed in the UI correctly, the name is not read and elements of the JSON itself show up in the UI like "core_path" or "db_name".

Steps to reproduce the bug

1. Use Scan Directory to create a playlist.
2. Close RetroArch and go into the playlist directory (~/snap/retroarch/188/.config/retroarch/playlists for me) and modify the playlist .lpl file.
3. Remove all whitespace, for example by running it through an online minifier.
4. Open RetroArch and look at the playlist.

Version/Commit

7ffeebf

• RetroArch: [version/commit]

Environment information

• OS: Ubuntu 18.04

[bparker06]

possible duplicate of #8379

[ZornTaov]

Indeed it's a related or duplicate issue, since if this is happening when you remove whitespaces then RA is hardcoding both reading and writing of files to expect they're in a certain format. This is not how JSON is supposed to work...

[fr500]

But did you read the whole thread?
Are you sure the whole json is wrong? or is it just the version string (we still favor old flavor playlists, it's just a backwards compatiblity method)

It needs to be fixed, but I have the feeling the issue is not as broad as it seems.

[fr500]

For instance I minified my history playlist:

{
  "version": "1.0",
  "items": [
    {
      "path": "W:\\roms\\Emulated\\Arcade\\Final Burn Alpha\\sfiii3.zip",
      "label": "",
      "core_path": "D:\\GameData\\Emulators\\RetroArch\\libretro\\fbalpha_libretro.dll",
      "core_name": "Arcade (FB Alpha)",
      "crc32": "",
      "db_name": ""
    },
    {
      "path": "W:\\roms\\Emulated\\Arcade\\Final Burn Alpha\\sfiii.zip",
      "label": "",
      "core_path": "D:\\GameData\\Emulators\\RetroArch\\libretro\\fbalpha_libretro.dll",
      "core_name": "Arcade (FB Alpha)",
      "crc32": "",
      "db_name": ""
    }
  ]
}

{"version":"1.0","items":[{"path":"W:\\roms\\Emulated\\Arcade\\Final Burn Alpha\\sfiii3.zip","label":"","core_path":"D:\\GameData\\Emulators\\RetroArch\\libretro\\fbalpha_libretro.dll","core_name":"Arcade (FB Alpha)","crc32":"","db_name":""},{"path":"W:\\roms\\Emulated\\Arcade\\Final Burn Alpha\\sfiii.zip","label":"","core_path":"D:\\GameData\\Emulators\\RetroArch\\libretro\\fbalpha_libretro.dll","core_name":"Arcade (FB Alpha)","crc32":"","db_name":""}]}

Of course this didn't work due to the aforementioned issue.
Then I changed the playlist code like this:

$ git diff
diff --git a/playlist.c b/playlist.c
index 6951464233..c3215532e6 100644
--- a/playlist.c
+++ b/playlist.c
@@ -1364,9 +1364,9 @@ static bool playlist_read_file(

       filestream_seek(file, 0, SEEK_SET);

-      if (bytes_read == 15)
+      if (bytes_read == 64)
       {
-         if (string_is_equal(buf, "{\n  \"version\": "))
+         if (strstr(buf,"version"))
          {
             /* new playlist format detected */
             /*RARCH_LOG("New playlist format detected.\n");*/

And it works,

Which means the whole json with exception of this small hiccup is compliant, of course fixes are welcome, but we must definitely not break backwards compatibility. Lots of people have spent a lot of time making nice playlists for their setups.

[ZornTaov]

Right, derped on that a little bit. If the fix really is that for both issues then it'd be great if it got PR'd in.

[fr500]

no, that fix would cause lots of false positives, that's why I haven't PR'd it.

[ZornTaov]

I suppose only checking for "{" at the first byte would also create false positives if, for whatever reason, the root folder the rom is in starts with a "{"?

[natinusala]

Another solution would be to add a marker on top of every playlist, like so

#playlist json
{
  "version": "1.0",
  "items": [
    {
      "path": "W:\\roms\\Emulated\\Arcade\\Final Burn Alpha\\sfiii3.zip",
      "label": "",
      "core_path": "D:\\GameData\\Emulators\\RetroArch\\libretro\\fbalpha_libretro.dll",
      "core_name": "Arcade (FB Alpha)",
      "crc32": "",
      "db_name": ""
    },
    {
      "path": "W:\\roms\\Emulated\\Arcade\\Final Burn Alpha\\sfiii.zip",
      "label": "",
      "core_path": "D:\\GameData\\Emulators\\RetroArch\\libretro\\fbalpha_libretro.dll",
      "core_name": "Arcade (FB Alpha)",
      "crc32": "",
      "db_name": ""
    }
  ]
}

#playlist legacy
whatever is inside a legacy playlist

Then any playlist parser can use that marker to identify the format, and then parse the playlist (without the marker, of course). Newly created playlists can have that, and old playlists can fallback to the current identification code.

edit: this also makes it way easier to create new playlist formats in the future

[fr500]

Adding it to the new ones would work and would be enough.
Adding to the old ones is pointless anyway since they get converted to new format.

[ofry]

Could we do check playlist format (old or new) by this way:

Just validate playlist file contents as JSON, as described http://rapidjson.org/md_doc_schema.html

If pass, then it has JSON.
If fail, then it has old format.

[natinusala]

Parsing the whole JSON is way too slow I think

[ZornTaov]

The JSON file would have to be loaded and parsed anyways if it was valid JSON, and if there's any error (malformed JSON or isn't JSON period) what the parser should do is exit immediately without trying to parse the rest of the file. So if it's in the old format it should fail after the first character or so (since it's not starting with a { or [ ) and then the file would be loaded in the old format. The only time it would be slower than normal is if the JSON was bad after loading, say, half the file and then it re-parses the entire file over again in the old format (which if it was supposed to be JSON would result in a malformed playlist rendered in the menu)

[geajack]

I'm a little confused at the talk of backwards compatibility in this thread. The format which RetroArch currently accepts in playlists is a subset of JSON, if I'm understanding correctly. This means any playlist which a user wrote which is accepted by the old parser will be accepted by a newer, JSON-compliant parser.

If we were to just take out the current parser and replace it all with calls to some JSON library, would it fail to parse "old-style" playlists? If so, I'm missing something. What would break?

[natinusala]

Old style playlists are a list of lines, each entry taking 5 lines I believe.

It's something like
Content name
Content path
Core association
Core path
Playlist name
[... Repeat]

So the JSON parser won't work at all with those. It's not a subset of JSON.

[ZornTaov]

Content name
Content path
Core association
Core path

six, you're missing the crc hash before the playlist name

Playlist name
[... Repeat]

[geajack]

@natinusala Ah, you're saying there's another playlist format which is even older than the current one?

[natinusala]

To my knowledge there are two playlist formats : the old one, which is a hardcoded 6 lines per entry, and the current one which is JSON.

[Ferk]

Parsing the whole JSON is way too slow I think

Not just the speed, another problem is the memory.
Playlists can be huge, someone could have thousands of games, so the JSON should be processed element by element, releasing memory after each element read and allow for pagination parsing a few elements at a time without having to keep the entire thing in memory.

This could be done in a simple way by using newline-delimited JSON (see http://jsonlines.org/ ), but that would require re-defining the format (plus it'd be uglier to edit by hand), so the most suitable approach would be using a SAX-like parser or something like this "bitwise JSON parser" is doing: https://web.cs.dal.ca/~sjackson/bitJson/JSON.html

Another solution would be to add a marker on top of every playlist

If something like this is done, please drop the "version" element, put the version in that marker if you need it. It makes very little sense to have a "version" in the json, imho. It would make sense in XML as an attribute of the tag, but json does not have attributes and you cannot force the order or position of the elements in an object and still consider it json. Also I'm not sure if version numbers are very useful in extensible formats, personally I'd rather use new fields for new features. Feature flags is a much more convenient and maintainable way to extend functionality, than having to carry over the historical baggage of which numeric version did what.

[geajack]

To my knowledge there are two playlist formats : the old one, which is a hardcoded 6 lines per entry, and the current one which is JSON.

Can you give an example of a playlist which would be parsed by the current code, but would fail to be parsed with a standard JSON parser? I'm struggling to see how that could be possible.

[RobLoach]

The problem is that we don't know whether the playlist is legacy or of JSON format. If we use a .json file extension, then there will be no question of which format it is. Have .lpl be the legacy playlist format, and .json files be the new ones.

playlists/Nintendo - Nintendo 64.lpl  # Old format
playlists/Nintendo - Nintendo Entertainment System.json  # New format

Alternatively, building upon @fr500's suggestion in https://github.com/libretro/RetroArch/issues/8439#issuecomment-471364318 , we could loosen the requirement check even further, and just look for { as the first character and assume it's JSON: https://github.com/libretro/RetroArch/pull/8489

[daliaetnano]

I suggest to get rid of the legacy format by providing a conversion script.
The format of the legacy format is so simple. A conversion script can be easily written in any scripting language: python, shell...

[bparker06]

@natinusala @Ferk The JSON playlists are already parsed one element at a time and it isn't kept in memory... the performance was also measured to be completely unnoticeable on the tested platforms vs the old format (I know Switch was one of them).

The only thing that's kept in memory is the final playlist_t structs that the JSON feeds into, which was always the case even with the old format.

[natinusala]

So nothing prevents us from doing this:

• Parse the playlist as JSON - if it succeeds, it means that it's the JSON format -> use it
• If it fails, parse it using the old format (or convert it) -> use it