Skip to content

Improvements to gbam C version - #21

Open
chris7716 wants to merge 19 commits into
NickRoz1:feature/c_versionfrom
chris7716:mac-issues
Open

chris7716 wants to merge 19 commits into
NickRoz1:feature/c_versionfrom
chris7716:mac-issues

Conversation

@chris7716

Copy link
Copy Markdown
Contributor

No description provided.

@chris7716
chris7716 changed the base branch from master to feature/c_version June 24, 2025 07:05
@NickRoz1

Copy link
Copy Markdown
Owner

why read record returns a pointer?

@chris7716

Copy link
Copy Markdown
Contributor Author

why read record returns a pointer?

Removed and made it void.

@NickRoz1

Copy link
Copy Markdown
Owner

i think i already wrote sam hdr there

Comment thread c_attempt/main.c
Comment on lines +109 to +111
htsFile *fp = hts_open("-", "w"); // "-" means stdout
sam_hdr_write(fp, reader->header);
hts_close(fp);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NickRoz1 Yes, you do include the sam hdr in the gbam. But I didn't see this when tryiing to read the gbam so that the output txt file also has the sam header. @AndreaGuarracino said that we need to have it in the output txt.

Comment thread c_attempt/reader.c
Comment on lines +72 to +77
// reader->header = sam_hdr_parse(strlen(file+seekpos+meta_size), file+seekpos+meta_size); <- this doesn't work
// because strlen(...) calculates length until the first \0 byte, which is not guaranteed in binary data.
int32_t *header_len_ptr = (int32_t *)(file + seekpos + meta_size);
char *header_start = (char *)(header_len_ptr + 1);
assert(*header_len_ptr == header_len); // Sanity check to confirm the calculated header length is matched with the header length stored in the metadata.
reader->header = sam_hdr_parse(*header_len_ptr, header_start);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NickRoz1 I think strlen(file+seekpos+meta_size) doesn't give the correct header length. I had to use (int32_t *)(file + seekpos + meta_size) get the correct header length.

I have also included header_len in the meta header so that we can do a sanity check when reading the gbam to check calculated header length is matched with the header length stored in the metadata.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you jump to the meta offset + metasize and then read till the end of the file you get the header
eventually we must also write crc32 in the end of the file so the header length info is not necessary
if crc32 of meta plus header wont match when reading then we have damage in the file

@chris7716 chris7716 changed the title Fix some build issues in the C code Improvements to gbam C version Jun 25, 2025
@NickRoz1

Copy link
Copy Markdown
Owner

Needs rebase plus why logs in PR?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants