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

Add a way to reduce the padding after writing tags #229

Closed
lazka opened this issue Jul 23, 2015 · 10 comments
Closed

Add a way to reduce the padding after writing tags #229

lazka opened this issue Jul 23, 2015 · 10 comments
Labels

Comments

@lazka
Copy link
Member

@lazka lazka commented Jul 23, 2015

Originally reported by: Philipp Wolfer (Bitbucket: phwolfer, GitHub: Unknown)


While in general not touching the padding is a good thing, sometimes it would be desirable to reduce the padding to save some space, e.g. after the user removed a larged, embedded image.

AFAIK mutagen does currently not provide an API for that.

See also discussion on metabrainz/picard#395


@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Jul 23, 2015

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


Agreed.

Maybe we can pass in a padding strategy across all formats and provide a
reasonable default implementation?

#!python
def padding_strategy(old_padding, new_padding):
    """Gives the padding present before the tag was written and the padding 
    that would be left after writing (can be negative if the tag size is too 
    big) should return the new (non-negative) padding size.
    """

    pass

def no_padding(*args):
    """This removes all padding in all cases"""

    return 0

tag.write(padding=no_padding)

Would that be OK? Any other suggestion?

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Jul 23, 2015

Original comment by Philipp Wolfer (Bitbucket: phwolfer, GitHub: Unknown):


That looks great and pretty flexible, better than just giving absolute values or percentages. Would allow us to easily hook in some preferences in Picard.

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Jul 23, 2015

Original comment by Philipp Wolfer (Bitbucket: phwolfer, GitHub: Unknown):


For a default implementation I would be rather conservative. Either go the no padding way or only pad if it makes a huge difference (several MB).

Would the total file be of interest for the padding strategy?

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Jul 23, 2015

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


Using the file size sounds like a good idea as resizing large files is slower and we should try to keep more padding there.

My suggestion:

  • If the tag gets larger and still fits don't add more padding.
  • If the final padding makes more than MAX(5%, 8kb) of the file reduce it to MAX(1%, 4kb)
  • If the tag doesn't fit anymore add MAX(1%, 4kb) padding.
@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Aug 26, 2015

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


Add API for specifying the amount of padding to use. See #229

This adds a new padding parameter to ID3.save() and AIFF.save() which
can get passed a callback which gets passed a PaddingInfo
object. The callback can then decide how much padding to use
when saving based on how much would be left and the size
of the file.

This also adds the ability for ID3/AIFF tags to shrink again.
The default implemention will remove padding if the padding
makes up more than ~2.5% of the file.

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Aug 28, 2015

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


flac: implement padding control. See #229

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Sep 1, 2015

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


asf: add padding support. Fixes #201

Also adds padding control; see #229

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Sep 8, 2015

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


This should be implemented now except for APEv2 (we always write them at the end) and OggFLAC (too complicated)

See https://mutagen.readthedocs.org/en/latest/tutorial.html#padding

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Sep 8, 2015

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


It would probably also be a good idea to kill padding on FileType.delete() calls.

Reopening until that is solved.

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Sep 9, 2015

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


fixed now

@lazka lazka closed this Apr 7, 2016
lazka added a commit that referenced this issue Apr 7, 2016
This adds a new padding parameter to ID3.save() and AIFF.save() which
can get passed a callback which gets passed a PaddingInfo
object. The callback can then decide how much padding to use
when saving based on how much would be left and the size
of the file.

This also adds the ability for ID3/AIFF tags to shrink again.
The default implemention will remove padding if the padding
makes up more than ~2.5% of the file.
lazka added a commit that referenced this issue Apr 7, 2016
lazka added a commit that referenced this issue Apr 7, 2016
Also adds padding control; see #229
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.