ogg icon indicating copy to clipboard operation
ogg copied to clipboard

Improve seek

Open TonalidadeHidrica opened this issue 4 years ago • 0 comments

Current PacketReader:seek_absgp has several issues:

  • It may misbehave if it encounters to a page where no packet ends ( #10 )
  • It does not check for the integrity of the captured 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.

TonalidadeHidrica avatar Feb 14 '21 13:02 TonalidadeHidrica