Add non-Qt base64 encode/decode methods #6432

Veratil · 2022-06-11T16:15:33Z

Throwing this out to provide non-Qt base64 encode/decode methods. Threw them in the lmms::base64 namespace for now.

Currently provided:

std::string encode(const std::string&);
std::string decode(const std::string&);
char* encode(const char*, const size_t);

If we want to keep the char* type function, then I can add the decode method for that. If not we can delete it.

Also, I'm thinking it would be better to use move semantics for returning the std::string, but I'm not too versed in that aspect and how to make it work correctly.

sakertooth · 2022-06-11T16:25:22Z

Also, I'm thinking it would be better to use move semantics for returning the std::string, but I'm not too versed in that aspect and how to make it work correctly.

Generally, I don't think you would return T&&. If T is returned, copy elision (or more specifically, RVO) should kick in.

From https://en.cppreference.com/w/cpp/language/return#Notes:
If expression is a prvalue, the result object is initialized directly by that expression. This does not involve a copy or move constructor when the types match (see [copy elision](https://en.cppreference.com/w/cpp/language/copy_elision)).

From the standard (This one had a nice example):
— A prvalue (“pure” rvalue) is an rvalue that is not an xvalue. [ Example: The result of calling a function whose return type is not a reference is a prvalue. The value of a literal such as 12, 7.3e5, or true is also a prvalue. —end example ]

LmmsBot · 2022-06-11T16:26:54Z

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

Linux (AppImage): lmms-1.3.0-alpha.1.216+g2bd9facd8-linux-x86_64.AppImage (build link)

Windows

Windows 32-bit: lmms-1.3.0-alpha.1.216+g2bd9facd8-mingw-win32.exe (build link)
Windows 64-bit: lmms-1.3.0-alpha.1.216+g2bd9facd8-mingw-win64.exe (build link)

macOS

macOS : lmms-1.3.0-alpha.1.216+g2bd9facd8-mac10.14.dmg (build link)

🤖

{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://output.circle-artifacts.com/output/job/91e9e3cd-c4c4-421b-944b-ddecd4031602/artifacts/0/lmms-1.3.0-alpha.1.216+g2bd9facd8-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17728?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://output.circle-artifacts.com/output/job/6eb04a28-5347-497a-a671-acc71e79d15e/artifacts/0/lmms-1.3.0-alpha.1.216+g2bd9facd8-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17727?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://output.circle-artifacts.com/output/job/d312eeea-8dcd-4c92-b8e0-98b950aff507/artifacts/0/lmms-1.3.0-alpha.1.216+g2bd9facd8-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17730?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://output.circle-artifacts.com/output/job/c7ddd18f-e1d9-4013-8538-d11ceeea3b5e/artifacts/0/lmms-1.3.0-alpha.1.216+g2bd9facd8-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17729?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "29ec4bfbc8f2209d18a2550ce918ba16692f793a"}

JohannesLorenz · 2022-06-16T18:41:38Z

What would be the reason to elliminate Qt methods? FWIR, this is only useful for the GUI, which uses Qt anyways?

Veratil · 2022-06-16T18:46:33Z

What would be the reason to elliminate Qt methods? FWIR, this is only useful for the GUI, which uses Qt anyways?

Good question! Here's the reason this PR exists:

It's in core, we're removing Qt from core.
This was easy enough to throw together.

If base64 is unneeded in core then we just close this PR and move the file out of core folder into gui instead. 🙂

sakertooth · 2022-06-16T19:20:40Z

What would be the reason to elliminate Qt methods? FWIR, this is only useful for the GUI, which uses Qt anyways?

FWIW, my sample caching PR utilizes Base64 encoding/decoding within SampleBufferV2 (eventually it will be just SampleBuffer) when loading data from Base64. As SampleBufferV2 is part of the core, this is a useful PR. This could've been part of the original Qt removal PR for simplicity's sake though.

Do note that this is one example of Base64 usage within the core. There is probably more.

PhysSong · 2022-06-18T03:17:34Z

I suggest always returning std::string and removing code duplication using std::string_view.

PhysSong · 2022-06-19T02:08:15Z

Well, I mean, you can merge two encode functions using std::string_view and optionally add std::string/const char*, size_t variants.

Veratil · 2022-06-19T02:54:08Z

Well, I mean, you can merge two encode functions using std::string_view and optionally add std::string/const char*, size_t variants.

Oh! I thought you were only talking about the std::string ones. 😁

sakertooth · 2022-06-25T15:00:00Z

I made some tweaks to this implementation here. Took me a while because I was constantly refactoring it and trying to get something compact. Both functions should be functional and were tested with the test vectors specified in RFC 4648 section 4.

JohannesLorenz · 2022-06-30T19:21:59Z

Suggesting to add a few simple "unit tests", because it should be easy to test and helps us verifying it works.

Veratil · 2022-07-02T02:21:46Z

Suggesting to add a few simple "unit tests", because it should be easy to test and helps us verifying it works.

Done.

Also changed the current Qt-based encode/decode namespace to lmms::gui::base64, and kept the new in lmms::base64. This way we can adjust things to the new functions as needed.

PhysSong · 2022-07-02T07:03:15Z

Also changed the current Qt-based encode/decode namespace to lmms::gui::base64

Why in lmms::gui? We're removing Qt from core, but the base64 methods has nothing to do with GUI.

Also for the unit tests, I suggest iterating over original-encoded pairs and check encoding/decoding in the loop body, if possible.

sakertooth · 2022-07-02T10:25:21Z

Also changed the current Qt-based encode/decode namespace to lmms::gui::base64, and kept the new in lmms::base64. This way we can adjust things to the new functions as needed.

Why not just just provide an overload for QString and put the Qt implementation there? No need for these widespread changes in my honest opinion.

Once QString has been removed in the Qt-removal PR, we can remove the Qt Base64 functions as well.

Veratil · 2022-07-02T13:16:21Z

Why in lmms::gui? We're removing Qt from core, but the base64 methods has nothing to do with GUI.

Whenever I left them in the same namespace I was getting errors about invalid overloads or something.
EDIT: I'm not getting them now. I don't know what was wrong before.

Also for the unit tests, I suggest iterating over original-encoded pairs and check encoding/decoding in the loop body, if possible.

They're just tests, but okay. :)

tests/src/core/base64Test.cpp

include/base64.h

src/core/base64.cpp

PhysSong · 2023-01-17T12:26:31Z

src/core/base64.cpp

+	auto b64char1 = [](std::string_view chunk) {
+		return static_cast<std::string::value_type>(chunk[0] >> 2);


Any reason for passing std::string_view by value?

Given that std::string is an alias of std::basic_string<char>, std::string::value_type is always char.

My understanding of string_view was that this is the way to do it. If not then I can change it. 👍

My thought was in the instance for some reason std::string isn't based on char.

My understanding of string_view was that this is the way to do it.

It turns out you're correct, indeed.

PhysSong · 2023-01-17T12:34:34Z

src/core/base64.cpp

+			case 3:
+				output[3] = map[b64char4(chunk)];
+			case 2:
+				output[2] = map[b64char3(chunk)];
+			default: /* no-op */;


It might be good to add [[fallthrough]]; when falling through. I prefer explicitly breaking on case 2 and default, but I'll leave it up to you.

I was not aware of this, I'll add them.

sakertooth · 2023-07-23T00:36:38Z

include/base64.h


-#endif
+namespace lmms::base64 {
+	constexpr inline std::array<char, 64> map =


constexpr implies inline, so inline should be redundant here.

That might not be true for global variables, but regardless, I think all of these implementation details do not belong in the header anyway and should be moved to base64.cpp. Then they certainly will not need the inline.

sakertooth · 2023-07-23T00:37:15Z

include/base64.h

+		'0','1','2','3','4','5','6','7','8','9',
+		'-','_'
+	};
+	const inline std::map<char, int> rmap {


Couple of things:

Why not use constexpr here as well? If anything, it will be consistent with map.

Couldn't we have used a better name, maybe reverseMap instead of rmap?

Subjective, but could we also add a space in between the preceding and succeeding constexpr variables? (Usually I wouldn't mind if they were each one line, but they span multiple)

Unfortunately, std::map does not have a constexpr constructor so rmap cannot be constexpr, but I agree with your other points.

sakertooth · 2023-07-23T00:41:28Z

include/base64.h

+	/*
+		This section of math ensures that base64 encode/decode will work
+		as intended. Some rare architectures don't use 8-bit char's, and
+		it's possible this won't work as intended if a char isn't 8-bits.
+
+		In the rare case this is ported to an architecture where this
+		happens, feel free to comment out the static_assert's and test.
+	*/
+	constexpr int char_bits = std::numeric_limits<std::string_view::value_type>::digits;
+	constexpr int sign_bit = std::numeric_limits<std::string_view::value_type>::is_signed ? 1 : 0;
+	// check that the string_view character type is 8 (7 signed + 1 sign) bits wide
+	static_assert(char_bits + sign_bit == 8);
+	constexpr int numBitsPerChar = char_bits + sign_bit;
+	constexpr int numBitsPerBase64Char = 6;
+	constexpr int lcm = std::lcm(numBitsPerChar, numBitsPerBase64Char);
+	// make sure math works, 24 bits
+	static_assert(lcm == 24);
+	constexpr int numBytesPerChunk = lcm / numBitsPerChar;
+	constexpr int numBase64CharPerChunk = lcm / numBitsPerBase64Char;
+	// double check math works and bit width matches
+	static_assert(numBytesPerChunk * numBitsPerChar == numBase64CharPerChunk * numBitsPerBase64Char);


I do like your attention towards portability, but a char not being 8 bits is very unlikely, especially given the architectures we are targeting. Even on architectures we aren't directly supporting (think ARM), char is still 8 bits.

This can be somewhat subjective, but the tradeoff for more code complexity over certain things that we can almost agree will certainly be the case, like a char being 8 bits, might not be worth it.

sakertooth · 2023-07-23T00:43:50Z

src/core/base64.cpp

+		);
+	};
+	for (int currentChunk = 0; currentChunk < numChunks.quot; ++currentChunk) {
+		std::string_view chunk = data.substr(currentChunk * numBase64CharPerChunk, numBase64CharPerChunk);


I think using auto here is a plus.

Suggested change

std::string_view chunk = data.substr(currentChunk * numBase64CharPerChunk, numBase64CharPerChunk);

auto chunk = data.substr(currentChunk * numBase64CharPerChunk, numBase64CharPerChunk);

sakertooth · 2023-07-23T00:49:22Z

tests/src/core/base64Test.cpp

+    void create_test_data()
+    {
+        QTest::addColumn<QString>("original");
+        QTest::addColumn<QString>("encoded");
+
+        // Test Vectors from RFC 4648 Section 10
+        QTest::newRow("empty string")  << ""       << "";
+        QTest::newRow("1 chunk 2 pad") << "f"      << "Zg==";
+        QTest::newRow("1 chunk 1 pad") << "fo"     << "Zm8=";
+        QTest::newRow("1 chunk 0 pad") << "foo"    << "Zm9v";
+        QTest::newRow("2 chunk 2 pad") << "foob"   << "Zm9vYg==";
+        QTest::newRow("2 chunk 1 pad") << "fooba"  << "Zm9vYmE=";
+        QTest::newRow("2 chunk 0 pad") << "foobar" << "Zm9vYmFy";
+    }
+    void b64_encode_data()
+    {
+        create_test_data();
+    }
+    void b64_encode()
+    {
+        using namespace lmms::base64;
+
+        QFETCH(QString, original);
+        QFETCH(QString, encoded);
+        QCOMPARE(QString(encode(original.toStdString()).c_str()), encoded);
+    }
+    void b64_decode_data()
+    {
+        create_test_data();
+    }
+    void b64_decode()
+    {
+        using namespace lmms::base64;
+
+        QFETCH(QString, original);
+        QFETCH(QString, encoded);
+        QCOMPARE(original, QString(decode(encoded.toStdString()).c_str()));
+    }


I would personally add spaces between function definitions.

sakertooth · 2023-07-23T01:42:21Z

tests/src/core/base64Test.cpp

+    {
+        create_test_data();
+    }
+    void b64_decode()


Suggested change

void b64_decode()

void b64Decode()

sakertooth · 2023-07-23T01:42:41Z

tests/src/core/base64Test.cpp

+    }
+    void b64_decode_data()
+    {
+        create_test_data();


Suggested change

create_test_data();

createTestData();

sakertooth · 2023-07-23T01:43:04Z

tests/src/core/base64Test.cpp

+    }
+    void b64_encode_data()
+    {
+        create_test_data();


Suggested change

create_test_data();

createTestData();

sakertooth · 2023-07-23T01:43:55Z

tests/CMakeLists.txt

 	$<TARGET_OBJECTS:lmmsobjs>

 	src/core/AutomatableModelTest.cpp
+	src/core/base64Test.cpp


Should be src/core/Base64Test.cpp.

sakertooth · 2023-07-23T01:44:47Z

tests/src/core/base64Test.cpp

@@ -0,0 +1,73 @@
+/*
+ * base64Test.cpp


Suggested change

* base64Test.cpp

* Base64Test.cpp

sakertooth · 2024-02-12T08:37:49Z

Hey @Veratil, I would like to fix the merge conflicts here and try implementing this again if you would allow it. Don't want #7095 to just be closed (its their first PR, so I would like to merge it at some point), so I'll leave the unit test for that PR.

Veratil force-pushed the base64 branch from fc9feee to 50c3619 Compare June 18, 2022 17:57

Veratil force-pushed the base64 branch from 50c3619 to f884b6c Compare July 2, 2022 02:19

Add non-Qt base64 encode/decode methods and tests

f80e0d9

Veratil force-pushed the base64 branch from f884b6c to f80e0d9 Compare July 2, 2022 14:19

Small updates

29ec4bf

sakertooth mentioned this pull request Jul 20, 2022

Implement sample caching #6390

Closed

PhysSong reviewed Jan 4, 2023

View reviewed changes

tests/src/core/base64Test.cpp Outdated Show resolved Hide resolved

messmerd requested changes Jan 8, 2023

View reviewed changes

include/base64.h Show resolved Hide resolved

include/base64.h Outdated Show resolved Hide resolved

src/core/base64.cpp Outdated Show resolved Hide resolved

src/core/base64.cpp Outdated Show resolved Hide resolved

Address review comments

7844205

PhysSong mentioned this pull request Jan 15, 2023

Refactor SampleBuffer #6610

Merged

PhysSong reviewed Jan 17, 2023

View reviewed changes

PhysSong marked this pull request as ready for review January 18, 2023 01:11

sakertooth reviewed Jul 23, 2023

View reviewed changes

TechnoPorg mentioned this pull request Feb 10, 2024

Add unit tests for Base64 encoding/decoding methods #7095

Closed

		auto b64char1 = [](std::string_view chunk) {
		return static_cast<std::string::value_type>(chunk[0] >> 2);

	std::string_view chunk = data.substr(currentChunk * numBase64CharPerChunk, numBase64CharPerChunk);
	auto chunk = data.substr(currentChunk * numBase64CharPerChunk, numBase64CharPerChunk);

Uh oh!

Add non-Qt base64 encode/decode methods #6432

Are you sure you want to change the base?

Add non-Qt base64 encode/decode methods #6432

Uh oh!

Conversation

Veratil commented Jun 11, 2022

Uh oh!

sakertooth commented Jun 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LmmsBot commented Jun 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Linux

Windows

macOS

Uh oh!

JohannesLorenz commented Jun 16, 2022

Uh oh!

Veratil commented Jun 16, 2022

Uh oh!

sakertooth commented Jun 16, 2022

Uh oh!

PhysSong commented Jun 18, 2022

Uh oh!

PhysSong commented Jun 19, 2022

Uh oh!

Veratil commented Jun 19, 2022

Uh oh!

sakertooth commented Jun 25, 2022

Uh oh!

JohannesLorenz commented Jun 30, 2022

Uh oh!

Veratil commented Jul 2, 2022

Uh oh!

PhysSong commented Jul 2, 2022

Uh oh!

sakertooth commented Jul 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Veratil commented Jul 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sakertooth commented Jun 11, 2022 •

edited

Loading

LmmsBot commented Jun 11, 2022 •

edited

Loading

sakertooth commented Jul 2, 2022 •

edited

Loading

Veratil commented Jul 2, 2022 •

edited

Loading