Add latest function to Metadata

[JasonLG1979]

• Gets an immutable reference to the latest, and therefore newest, revision of the metadata.

Removes the need to pop though the revisions via a loop when all you want is the latest.

As mentioned in librespot-org/librespot#967

[pdeljanov]

Hi @JasonLG1979,

I commented on your linked issue, but I'll copy and paste the parts relevant to this PR below.

The interface is purposefully designed to make a user iterate over the revisions for two reasons. The first is to get them to pop off old revisions from the queue to prevent a pseudo memory leak. The second is because Symphonia doesn't try to guess how metadata should be used. For example, a revision could be a completely new set of tags or it could only contain an updated set of tags. It is an application specific decision, though I'd wager the former is the case the majority of the time.

Unfortunately, as-is this PR will expose a footgun in the form of a pseudo memory leak, as well as potentially weird behaviour in the latter case mentioned in the comment above.

I'm open to making life easier for your use-case though.

Perhaps we could modify the behaviour of the function to clear all but the latest revision. Then rename the function to clear_old, clear_previous, pop_previous, or something to better explain what it's actually doing.

[JasonLG1979]

Unfortunately, as-is this PR will expose a footgun in the form of a pseudo memory leak, as well as potentially weird behaviour in the latter case mentioned in the comment above.

I'm open to making life easier for your use-case though.

Perhaps we could modify the behaviour of the function to clear all but the latest revision. Then rename the function to clear_old, clear_previous, pop_previous, or something to better explain what it's actually doing.

Ok. That sounds like it complicates things. A person could for example assume that rev2 contains all of rev1 + whatever changed from rev1 to rev2 and that doesn't always seem to be the case. If the revs can be implemented as basically diffs then we may need to change our implementation at librespot and just fast forwarding to the latest without considering prev revs is a mistake.

[pdeljanov]

I think for librespot it's perfectly okay to assume the last revision is the latest and complete. I wouldn't recommend any changes other than properly popping queue. That's because Spotify just serves you a single song per file. Even in the case of something like a chained OGG physical stream where new revisions would be expected, the diffing use-case would be very application specific. I just don't want Symphonia to assume how a user should consume their metadata.

[JasonLG1979]

I think for librespot it's perfectly okay to assume the last revision is the latest and complete. I wouldn't recommend any changes other than properly popping queue.

In my PR to librespot I do:

        let latest_metadata = if metadata.is_latest() {
            metadata.current()?
        } else {
            loop {
                match metadata.pop() {
                    // Advance to the latest metadata revision.
                    Some(_discarded_revision) => continue,
                    // None means we hit the latest.
                    None => break metadata.current()?,
                }
            }
        };

Is that not correct?

Even in the case of something like a chained OGG physical stream where new revisions would be expected, the diffing use-case would be very application specific.

That's all Greek to me. I will take your word for it.

I just don't want Symphonia to assume how a user should consume their metadata.

That's completely understandable.

[JasonLG1979]

Perhaps we could modify the behaviour of the function to clear all but the latest revision. Then rename the function to clear_old, clear_previous, pop_previous, or something to better explain what it's actually doing.

I'm not a fan of pop_* unless something is returned and pop_previous sounds like it should return current -1. clear_previous is also ambiguous IMHO for the same reason. Maybe something like fastforward_to_latest?

    pub fn fastforward_to_latest(&self) {
        let rev_len = self.revisions.len();
        if rev_len > 1 {
            self.revisions = self.revisions.split_off(rev_len - 1);
        }
    }

or

    pub fn fastforward_to_latest(&self) {
        loop {
            if self.pop().is_none() {
                break;
            }
        }
    }

[pdeljanov]

Hi @JasonLG1979,

In my PR to librespot I do:

        let latest_metadata = if metadata.is_latest() {
            metadata.current()?
        } else {
            loop {
                match metadata.pop() {
                    // Advance to the latest metadata revision.
                    Some(_discarded_revision) => continue,
                    // None means we hit the latest.
                    None => break metadata.current()?,
                }
            }
        };

Is that not correct?

This looks correct. Since that code is in the context of a larger function you could probably simplify it with something like this until the helper function in Symphonia gets published:

while metadata.pop().is_some() {}
let latest_metadata = metadata.current()?;

For this PR I like your second option. My only concern is that fast-forward has other meanings in media so I'd probably prefer just calling it skip_to_latest.

pub fn skip_to_latest(&mut self) {
    loop {
        if self.pop().is_none() {
            break;
        }
    }
}

Thanks!

[JasonLG1979]

This looks correct. Since that code is in the context of a larger function you could probably simplify it with something like this until the helper function in Symphonia gets published:

while metadata.pop().is_some() {}
let latest_metadata = metadata.current()?;

We are basically of one mind. Since my last comment I've changed my code in the librespot PR to basically the same as skip_to_latest.

loop {
    if metadata.pop().is_none() {
        break;
    }
}

For this PR I like your second option. My only concern is that fast-forward has other meanings in media so I'd probably prefer just calling it skip_to_latest.

Skip to last it is then.

[JasonLG1979]

Two more questions: Should skip_to_latest return the latest or not? And do you prefer the loop or while version? I know there is not functional difference, but your lib your choice. So...

1:

    pub fn skip_to_latest(&self) -> Option<&MetadataRevision> {
        loop {
            if self.pop().is_none() {
                break;
            }
        }
        self.revisions.pop_front()
    }

2:

    pub fn skip_to_latest(&self) -> Option<&MetadataRevision> {
        while self.pop().is_some() {}
        self.revisions.pop_front()
    }

3:

    pub fn skip_to_latest(&self) {
        loop {
            if self.pop().is_none() {
                break;
            }
        }
    }

4:

    pub fn skip_to_latest(&self) {
        while self.pop().is_some() {}
    }

[pdeljanov]

Options 1 and 2 are actually incorrect because it should be impossible to empty the underlying queue (Metadata::pop returns None when there is only 1 revision left, calling pop_front would then clear the queue). The reason being that the latest metadata should always be accessible.

But your suggestion is a good one. How about:

pub fn skip_to_latest(&self) -> Option<&MetadataRevision> {
    loop {
        if self.pop().is_none() {
            break;
        }
    }
    self.current()
}

Then a user could do:

let latest_metadata = metadata.skip_to_latest();

[JasonLG1979]

Options 1 and 2 are actually incorrect because it should be impossible to empty the underlying queue

Yep, that was a typo. My bad.

How's that look?

[pdeljanov]

Aside from the missing mut, it looks good to me. Once it's fixed I'll merge it!

[JasonLG1979]

More typos 🤦‍♂️

Done.

[pdeljanov]

We're all merged now. Thanks for your work on this!

[JasonLG1979]

Thank you!!!