Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve seek #22

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

TonalidadeHidrica
Copy link
Contributor

Current PacketReader:seek_absgp has several issues:

  • It may misbehave if it encounters to a page where no packet ends ( seek_absgp may not work properly when it encounters to a packet not containing the packet end #10 )
  • It does not check for the integrity of the captured page ( UntilPageHeaderReader may capture a position where it's not actually a beginning of a page #15 )
  • I'm not sure, but it seems that it seeks to the second packet of the appropriate page, not the first one
  • We cannot specify the range for searching, so it may not work for chained streams
  • The packet will have the absgp greater than or equal to the specified one. However, it doesn't have to be inclusive, because when we want the n-th sample, we don't have to care about the packets that completes the n-th sample.
  • The function does not provide the absgp for the packet before the seeked position, so it is hard to know the actual index of samples that are retrieved right after the seek.
  • The code uses a lot of macro and redundant, so it is hard to read and maintain.

My new implementation, PacketReader::seek_absgp_new handles all of those issues above. Also, I provided a new function find_end_of_logical_stream to find the end of a stream within multiple chained streams.

I know there are still some issues:

  • The new functions sare not well-tested yet. I've done some informal testing in this gist. Here, I tested find_next_page, my new seek_absgp_new function, and find_end_of_logical_stream function in test_find_next_page, test_seek, and test_find_end, respectively.
    • The issue is that I tested only against a file that contains single vorbis stream. Although I think my implementation works for chained and multiplexed streams, but it's not yet verified. Do you know such good files for testing purpose?
    • Should I add a test to this repository, rather than an external informal gist?
  • I haven't formatted the code yet. I know you don't like rustfmt, so I just kept as it is after I wrote the code. Thus there are even mixed tabs and spaces for indents. It's quite a hard work to do such formatting, so I'd like to do later, maybe right before merging.
  • I haven't tidied up the commits yet. I'd like to do this right before merge as well.

Other than those tasks, do you have any concerns about this PR? Feel free to tell me and I'll do my best.

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.

1 participant