LibAudio: Update FlacLoader comments and links to RFC 9639 #25940
Conversation
github-actions
bot
added
the
👀 pr-needs-review
|
@kleinesfilmroellchen you probably want to give it a look |
|
You need to run clang-format, see CI results. |
nico
added
the
⏳ pr-waiting-for-author
Updated all references to FLAC specifications within FlacLoader.h and FlacLoader.cpp to point to the official RFC 9639. This includes updating section numbers, direct links, and notes about previously referenced draft specifications. Fixes SerenityOS#25825.
Kenxpx
force-pushed
the
fix-flacloader-rfc9639
branch
from
e1ae09c to
c608f32
Compare
github-actions
bot
removed
the
⏳ pr-waiting-for-author
|
i ran clang-format but it still giving me same errors |
There was a problem hiding this comment.
So I just wanted to look up whether the quotes are still accurate, and at least according to my RFC 9639, your section numbers are entirely wrong. For example, the STREAMINFO block is described in 8.2 (https://www.rfc-editor.org/rfc/rfc9639#name-streaminfo), not 4.1., or UTF-8 numbers are 9.1.5 (https://www.rfc-editor.org/rfc/rfc9639#name-coded-number), not 5.1.7. What happened, and are you reading https://www.rfc-editor.org/rfc/rfc9639 ?
| #define FLAC_VERIFY(check, category, msg) \ | ||
| do { \ | ||
| if (!(check)) { \ | ||
| #define FLAC_VERIFY(check, category, msg) \ | ||
| do { \ | ||
| if (!(check)) { \ | ||
| return LoaderError { category, TRY(m_stream->tell()), TRY(String::formatted("FLAC header: {}", msg)) }; \ | ||
| } \ | ||
| } \ |
There was a problem hiding this comment.
Run your formatter again? This code shouldn’t have changed.
| @@ -81,7 +81,7 @@ MaybeLoaderError FlacLoaderPlugin::parse_header() | |||
| FixedMemoryStream streaminfo_data_memory { streaminfo.data.bytes() }; | |||
| BigEndianInputBitStream streaminfo_data { MaybeOwned<Stream>(streaminfo_data_memory) }; | |||
|
|
|||
| // 11.10 METADATA_BLOCK_STREAMINFO | |||
| // RFC 9639, Section 4.1: STREAMINFO Block | |||
There was a problem hiding this comment.
No need to say the RFC number every time, once per file is fine.
| @@ -167,7 +167,7 @@ MaybeLoaderError FlacLoaderPlugin::load_picture(FlacRawMetadataBlock& block) | |||
| if (offset_before_seeking + mime_string_length >= block.data.size()) | |||
| return LoaderError { LoaderError::Category::Format, TRY(m_stream->tell()), "Picture MIME type exceeds available data"_fly_string }; | |||
|
|
|||
| // "The MIME type string, in printable ASCII characters 0x20-0x7E." | |||
| // "The MIME type string, in printable ASCII characters 0x20-0x7E." (RFC 9639, Section 4.6) | |||
There was a problem hiding this comment.
This quote has changed
| // Note: This loader was originally written referencing older draft specifications | ||
| // (such as draft-ietf-cellar-flac-02) and xiph.org documentation. | ||
| // All spec-related comments and section number references should now be updated | ||
| // to align with RFC 9639. |
There was a problem hiding this comment.
This comment is entirely superflous, provided the section numbers are now all correct. The FLAC format has not changed between draft 2 and the RFC (in fact, the last incompatible change was made in 2007 and the last compatible change about a year later iirc)
nico
added
⏳ pr-waiting-for-author
LibAudio: Update FlacLoader comments and links to RFC 9639
Updated all references to FLAC specifications within FlacLoader.h
and FlacLoader.cpp to point to the official RFC 9639. This
includes updating section numbers, direct links, and notes
about previously referenced draft specifications.
Fixes #25825.