Construct upgrade to 2.10 #9
Conversation
src/pymp4/parser.py
Outdated
| @@ -767,7 +765,8 @@ def __init__(self, subcon): | |||
| self.flagbuildnone = True | |||
|
|
|||
| def _parse(self, stream, context, path): | |||
| return stream.tell() - self.subcon.sizeof(context) | |||
| import pdb;pdb.set_trace() | |||
There was a problem hiding this comment.
left in debugging code?
| Const(Int16ub, 0), | ||
| Const(Int32ub, 0), | ||
| Const(Int32ub, 0), | ||
| Padding(10), |
There was a problem hiding this comment.
Unnecessary change. It is more correct to go with the previous approach if its specific offsets/lengths are reserved. You also shouldn't keep the comment if you make this change.
src/pymp4/parser.py
Outdated
| @@ -84,22 +84,22 @@ def _sizeof(self, context, path): | |||
|
|
|||
| FileTypeBox = Struct( | |||
| "type" / Const(b"ftyp"), | |||
| "major_brand" / PaddedString(4, "utf8"), | |||
| "major_brand" / PaddedString(4, "ascii"), | |||
There was a problem hiding this comment.
Why replace explicitly "utf8" encodings with ascii? This is done various times in this commit and likely shouldn't be.
| Box = Prefixed(Int32ub, Struct( | ||
| "offset" / Tell, | ||
| "type" / Peek(PaddedString(4, "ascii")), | ||
| "box_body" / Switch(this.type, { |
There was a problem hiding this comment.
why "box_body"? Just leave as-is instead
There was a problem hiding this comment.
After 2 years I can´t remember much of this :) but I remember this being one of the main point. Construct having removed "embedded", it seemed to me the only way to do that would be to create a named "box_body" but of course it's very annoying to use and unnecessary. What would you suggest to avoid this exactly?
tests/test_box.py
Outdated
| @@ -25,13 +25,14 @@ | |||
|
|
|||
| class BoxTests(unittest.TestCase): | |||
| def test_ftyp_parse(self): | |||
| import pdb;pdb.set_trace() | |||
There was a problem hiding this comment.
Another left-in debug statement?
Hi,
despite being new to pymp4 and construct, I've decided to give a shot to upgrading to 2.10.60.
It was a useful learning exercise for me but before I spend any more effort on it, I wanted to get in touch with you. I'm sure you have considered this upgrade in the past and you probably have useful pointers or reason to do/not do it.
In this merge request, I've just focused on introducing as few modifications as possible to pymp4 struct definitions, and have all the included tests pass. I've made no efforts to refactor the struct definitions or to investigate new features available in Construct 2.10, nor in making sure the tests cover all changes, nor I have attempted to parse any "real" mp4. For the moment, I just wanted to understand the impacts of such an upgrade.
The removal of "Embedded" (surely done for good reasons) in particular is in my opinion a bit detrimental to the overall usability of the "Box" and "ContainerBox" structs, with the introduction of unnecessary fields whose presence is only needed to handle the struct nesting. You can immediately spot that just from the changes in the tests source code... unless there is a better way to replace it that I did not understand.
As the author, I'm sure you'll immediately know if this is something worth working on or not.
Thanks, let me know if you can