futzu / SCTE35_threefive

threefive is the one you want. threefive is the best SCTE-35 tool available. SCTE-35 Decoder, SCTE-35 Encoder, SCTE-35 Parser.
https://slow.golf
MIT License
136 stars 26 forks source link

Infinite loop in stream.py::_unpad() #101

Closed marksaxer-cbs closed 1 month ago

marksaxer-cbs commented 1 month ago

Hi Adrian,

I wanted to bring to your attention a loop that does not terminate.

The call stack is

_unpad, stream.py:445  <- recursive function call
_unpad_afc, stream.py:438
_parse_payload, stream.py:454
_parse_pts, stream:407
_parse, stream.py:493
decode_next, stream: 253

The cause, in my use case, is initializing a Stream() object with a .mp4 segment instead of a .ts

How to reproduce:

% threefive http://commondatastorage.googleapis.com/gtv-videos-bucket/sample/ElephantsDream.mp4 

# PAT changed @ 94287.385522

# PAT changed @ 94287.385522

# PAT changed @ 94287.385522

# PAT changed @ 94287.385522

# PAT changed @ 94287.385522

# PAT changed @ 94287.385522

# PAT changed @ 24306.266367

# PAT changed @ 24306.266367

# PAT changed @ 24306.266367

# PAT changed @ 24306.266367

^C%
futzu commented 1 month ago

Sorry, threefive doesn't do mp4 files.

You can't store SCTE-35 in mp4 last time I checked......

futzu commented 1 month ago

I am not big fan of recursion, but I'm getting padding put on the front and the back of the payload, and it's the best thing I can come up with to handle it. People are doing some weird stuff with packets lately, it's keeping me on my toes.

marksaxer-cbs commented 1 month ago

All good. FWIW, I was not expecting the .mp4 to contain SCTE-35s. In prior versions, threefive would immediately return if a .mp4 was provided. I recently upgraded from 2.3.81 to 2.4.57 and a function that performed a threefive check on each segment in an HLS manifest needed an update to conditionally check the segment type.

Feel free to close this issue.

Thanks!

futzu commented 1 month ago

I see now. I hate that unpad , but the problem is that ffmpeg is now altering SCTE-35 packets, and I'm trying to compensate for that, let me think on it for a minute, and I'm sure I can figure out how to handle that, because that is appropriate behavior, it should just return. Let's leave this open for now, I need to fix that. This is a bug.

futzu commented 1 month ago

FYI, I wasn't being a smartass about SCTE-35 and MP4, because there is a push to get it in mp4s, I wasn't sure if that had happened yet or not

futzu commented 1 month ago

I got a fix for you, let me test it a bit and I'll try to do a new release tonight.

futzu commented 1 month ago

I just pushed a new release, v2.4.67 and it should work for you, I tested it against the Elephant file. pip it up, and let me know how it goes.

marksaxer-cbs commented 1 month ago

Success! Thank you. Stream::decode_next() function now returns an IndexError if the Stream() was initialized with a .mp4

futzu commented 1 month ago

I didn't know you were using decode_next, I just did decode. I can make decode_next just return without an error on an mp4 I just have to add one line. Would that be better for you?

futzu commented 1 month ago
    def decode_next(self):
        """
        Stream.decode_next returns the next
        SCTE35 cue as a threefive.Cue instance.
        """
        if self._find_start():
            for pkt in self.iter_pkts():
                if pkt[0] == self.SYNC_BYTE:     # <--- I just need to add this line to avoid throwing an error
                    cue = self._parse(pkt)            
                    if cue:
                        return cue
        return False
futzu commented 1 month ago

Actually, I would like to make that change, unless it going to cause you problems. Let me know.

futzu commented 1 month ago

v2.4.69 returns without error on mp4 files with Stream.decode_next()