Rework segment.key as segment.keys, a list of keys #313

rlaphoenix · 2023-03-09T21:24:36Z

This closes #283 as it now follows RFC and keeps all preceding keys for the segment.

m3u8_obj.keys still has the same behaviour.
m3u8_obj.segments[0].key is now removed.
m3u8_obj.segments[0].keys is added in it's place, which will now always be a list.
The list will always be present, it will never be None anymore, and if the segment explicitly has no DRM by specifying an EXT-X-KEY of METHOD=NONE then it will be a list with that key.
It does not check if there's a METHOD=NONE with other conflicting EXT-X-KEY information.
It still follows the same behaviour of not duplicating the EXT-X-KEY information on future segments when dumping.

Do note that it only clears the list of Keys when it reaches an EXT-X-DISCONTINUITY, or it has reached a ts segment.

The copy() statement is used because it clears after the segment is appended to data, yet the .clear() to current_keys follows in the appended segment unless we copy that data in memory prior.

I don't think there's a need for .by_key() or m3u8_obj.keys anymore, but I've made sure those still work as intended in case there's a different reason those exist.

P.S., The by_key function does not work to match a key to a segment, therefore it's more or less useless. Similarly, the m3u8 keys list was on the right track but is ultimately useless as we cannot calculate which of the keys are for a segment. You cannot assume keys[0] is for segments[0].

The approach seen in this pull request is the only viable option to truly match EXT-X-KEYs to a segment when there are multiple preceding keys.

This closes globocom#283 as it now follows RFC and keeps all preceding keys for the segment. - `m3u8_obj.keys` still has the same behaviour. - `m3u8_obj.segments[0].key` is now removed. - `m3u8_obj.segments[0].keys` is added in it's place, which will now always be a list. - The list will always be present, it will never be `None` anymore, and if the segment explicitly has no DRM by specifying an EXT-X-KEY of METHOD=NONE then it will be a list with that key. - It does not check if there's a METHOD=NONE with other conflicting EXT-X-KEY information. - It still follows the same behaviour of not duplicating the EXT-X-KEY information on future segments when dumping. Do note that it only clears the list of Keys when it reaches an EXT-X-DISCONTINUITY, or it has reached a ts segment. The copy() statement is used because it clears after the segment is appended to `data`, yet the .clear() to `current_keys` follows in the appended `segment` unless we copy that data in memory prior. I don't think there's a need for `.by_key()` or `m3u8_obj.keys` anymore, but I've made sure those still work as intended in case there's a different reason those exist.

mauricioabreu · 2023-03-17T12:05:19Z

Hey @rlaphoenix sorry for the late reply. I will review it this weekend

rlaphoenix · 2023-03-17T18:37:15Z

Ok great. One thing I have thought of is we could keep compatibility by re-adding back key, and have it be the last entry of keys, sort of like how it works currently in master. That way current code expecting key that understood its flaw will still work and can gracefully migrate to using keys. Maybe mark it as deprecated and remove it 6 months down the line.

mauricioabreu · 2023-03-19T23:09:47Z

@rlaphoenix good job!

About implementing backwards compatibility, I think we can upgrade the major version of m3u8, because a major is expected to break clients that don't follow the new changes.

mauricioabreu · 2023-03-26T06:47:32Z

@rlaphoenix it looks like the test suite failed. Can you check it?

This keeps compatibility and fixes a lot of the existing tests. However, new tests should likely be implemented for testing HLS playlists with multiple preceeding keys.

rlaphoenix · 2023-03-26T09:37:10Z

Hi, apologies for all the squashes, but I believe it has the tests fixed. I have re-implemented segment.key for a more graceful deprecation procedure. Re-implementing it has effectively fixed a lot of the tests. However, that means keys has no tests, not really anyway. I don't have the best experience making tests, so any guidance on that would be appreciated.

Also, the tests on Windows causes it to compare e.g., /foo/bar with \foo\bar or \\foo\\bar, etc. Therefore I currently have 10 failed tests, most being caused by that. However, I also have a few tests related to by_key() failing.

============================================================================== short test summary info =============================================================================== 
FAILED tests/test_loader.py::test_load_should_create_object_from_file_with_relative_segments - AssertionError: assert 'C:\\Users\\Justin\\Projects\\m3u8\\tests/key.bin' == 'C:\\Users\\Justin\\Projects\\m3u8\\tests\\key.bin'
FAILED tests/test_loader.py::test_load_should_create_object_from_uri_with_relative_segments - AssertionError: assert 'http://localhost:8112\\path/key.bin' == 'http://localhost:8112/path/key.bin'
FAILED tests/test_model.py::test_keys_on_simple_encrypted_playlist - assert 2 == 1
FAILED tests/test_model.py::test_dump_should_raise_if_create_sub_directories_fails - Failed: DID NOT RAISE <class 'OSError'>
FAILED tests/test_model.py::test_length_segments_by_key - assert 0 == 2
FAILED tests/test_model.py::test_list_segments_by_key - AssertionError: assert '' == '../../../../hls/streamNum82400.ts\n../../../../hls/streamNum82401.ts'
FAILED tests/test_model.py::test_replace_segment_key - assert '#EXTM3U\n#EXT-X-MEDIA-SEQUENCE:82400\n#EXT-X-ALLOW-CACHE:NO\n#EXT-X-VERSION:2\n#EXT-X-TARGETDURATION:8\n#EXTINF:8,\n../../../../hls/streamNum82400.ts\n#EXTINF:8,\n../../....
FAILED tests/test_model.py::test_m3u8_should_propagate_base_uri_to_segments - AssertionError: assert '/any/path/entire1.ts' == '\\any\\path\\entire1.ts'
FAILED tests/test_model.py::test_m3u8_should_propagate_base_uri_to_key - AssertionError: assert '/any/key.bin' == '\\any\\key.bin'
FAILED tests/test_model.py::test_m3u8_should_propagate_base_uri_to_session_key - AssertionError: assert '/any/key.bin' == '\\any\\key.bin'
========================================================= 10 failed, 219 passed, 1 skipped, 9 xfailed, 2 warnings in 11.75s ==========================================================

Half of those failures seem to be Windows-specific, and not really an issue in practice. Here are logs of the other 5 errors in top to bottom order:

_______________________________________________________________________ test_keys_on_simple_encrypted_playlist _______________________________________________________________________ 

    def test_keys_on_simple_encrypted_playlist():
        obj = m3u8.M3U8(playlists.PLAYLIST_WITH_ENCRYPTED_SEGMENTS)

>       assert len(obj.keys) == 1
E       assert 2 == 1
E        +  where 2 = len([<m3u8.model.Key object at 0x000001FC795203A0>, None])
E        +    where [<m3u8.model.Key object at 0x000001FC795203A0>, None] = <m3u8.model.M3U8 object at 0x000001FC79520790>.keys

tests\test_model.py:240: AssertionError

Doesn't seem to be caused by my changes. At least not directly. The behavior seen here seems correct and keys should be a length of 2.

_______________________________________________________________ test_dump_should_raise_if_create_sub_directories_fails _______________________________________________________________ 

tmpdir = local('C:\\Users\\Justin\\AppData\\Local\\Temp\\pytest-of-Justin\\pytest-26\\test_dump_should_raise_if_crea0')

    def test_dump_should_raise_if_create_sub_directories_fails(tmpdir):
        # The first subdirectory is read-only
        subdir_1 = os.path.join(tmpdir, 'subdir1')
        os.mkdir(subdir_1, mode=0o400)
    
        # The file is to be stored in a second subdirectory that's underneath the first
        subdir_2 = os.path.join(subdir_1, 'subdir2')
        file_name = os.path.join(subdir_2, 'playlist.m3u8')

        # When we try to write it, we'll be prevented from creating the second subdirectory
>       with pytest.raises(OSError):
E       Failed: DID NOT RAISE <class 'OSError'>

tests\test_model.py:629: Failed

Seems like this may or may not be a windows thing, I'm not entirely sure what this is really checking.

____________________________________________________________________________ test_length_segments_by_key _____________________________________________________________________________ 

    def test_length_segments_by_key():
        obj = m3u8.M3U8(playlists.PLAYLIST_WITH_MULTIPLE_KEYS_UNENCRYPTED_AND_ENCRYPTED)

>       assert len(obj.segments.by_key(obj.keys[0])) == 2
E       assert 0 == 2
E        +  where 0 = len([])
E        +    where [] = <bound method SegmentList.by_key of [<m3u8.model.Segment object at 0x000001FC79E0CDC0>, <m3u8.model.Segment object at 0x000001FC79E0CCA0>, <m3u8.model.Segment
 object at 0x000001FC79E0CFA0>, <m3u8.model.Segment object at 0x000001FC79E0CF40>, <m3u8.model.Segment object at 0x000001FC79E0CC40>, <m3u8.model.Segment object at 0x000001FC79E0CF70>, <m3u8.model.Segment object at 0x000001FC79E0CFD0>, <m3u8.model.Segment object at 0x000001FC79E0DA20>]>(None)
E        +      where <bound method SegmentList.by_key of [<m3u8.model.Segment object at 0x000001FC79E0CDC0>, <m3u8.model.Segment object at 0x000001FC79E0CCA0>, <m3u8.model.Segment ob
ject at 0x000001FC79E0CFA0>, <m3u8.model.Segment object at 0x000001FC79E0CF40>, <m3u8.model.Segment object at 0x000001FC79E0CC40>, <m3u8.model.Segment object at 0x000001FC79E0CF70>, <
m3u8.model.Segment object at 0x000001FC79E0CFD0>, <m3u8.model.Segment object at 0x000001FC79E0DA20>]> = [<m3u8.model.Segment object at 0x000001FC79E0CDC0>, <m3u8.model.Segment object 
at 0x000001FC79E0CCA0>, <m3u8.model.Segment object at 0x000001FC79E0CFA0>, <m3u8.model.Segment object at 0x000001FC79E0CF40>, <m3u8.model.Segment object at 0x000001FC79E0CC40>, <m3u8.model.Segment object at 0x000001FC79E0CF70>, <m3u8.model.Segment object at 0x000001FC79E0CFD0>, <m3u8.model.Segment object at 0x000001FC79E0DA20>].by_key
E        +        where [<m3u8.model.Segment object at 0x000001FC79E0CDC0>, <m3u8.model.Segment object at 0x000001FC79E0CCA0>, <m3u8.model.Segment object at 0x000001FC79E0CFA0>, <m3u8
.model.Segment object at 0x000001FC79E0CF40>, <m3u8.model.Segment object at 0x000001FC79E0CC40>, <m3u8.model.Segment object at 0x000001FC79E0CF70>, <m3u8.model.Segment object at 0x000001FC79E0CFD0>, <m3u8.model.Segment object at 0x000001FC79E0DA20>] = <m3u8.model.M3U8 object at 0x000001FC79E0CAC0>.segments

tests\test_model.py:808: AssertionError

Like the first error log above, seems like it's testing the same, but with more variance of keys, therefore I still think it's not caused by my changes.

______________________________________________________________________________ test_replace_segment_key ______________________________________________________________________________ 

    def test_replace_segment_key():
        obj = m3u8.M3U8(playlists.PLAYLIST_WITH_MULTIPLE_KEYS_UNENCRYPTED_AND_ENCRYPTED)

        # Replace unencrypted segments with new key
        new_key = Key("AES-128", None, "/hls-key/key0.bin", iv="0Xcafe8f758ca555115584bb5b3c687f52")
        for segment in obj.segments.by_key(None):
            segment.key = new_key

        # Check dump
        expected = playlists.PLAYLIST_WITH_MULTIPLE_KEYS_UNENCRYPTED_AND_ENCRYPTED_UPDATED.strip()

>       assert obj.dumps().strip() == expected
E       assert '#EXTM3U\n#EXT-X-MEDIA-SEQUENCE:82400\n#EXT-X-ALLOW-CACHE:NO\n#EXT-X-VERSION:2\n#EXT-X-TARGETDURATION:8\n#EXTINF:8,\n../../../../hls/streamNum82400.ts\n#EXTINF:8,\n../.
./../../hls/streamNum82401.ts\n#EXT-X-KEY:METHOD=AES-128,URI="/hls-key/key.bin",IV=0X10ef8f758ca555115584bb5b3c687f52\n#EXTINF:8,\n../../../../hls/streamNum82400.ts\n#EXTINF:8,\n../..
/../../hls/streamNum82401.ts\n#EXTINF:8,\n../../../../hls/streamNum82402.ts\n#EXTINF:8,\n../../../../hls/streamNum82403.ts\n#EXT-X-KEY:METHOD=AES-128,URI="/hls-key/key2.bin",IV=0Xcafe
8f758ca555115584bb5b3c687f52\n#EXTINF:8,\n../../../../hls/streamNum82404.ts\n#EXTINF:8,\n../../../../hls/streamNum82405.ts' == '#EXTM3U\n#EXT-X-MEDIA-SEQUENCE:82400\n#EXT-X-ALLOW-CACH
E:NO\n#EXT-X-VERSION:2\n#EXT-X-TARGETDURATION:8\n#EXT-X-KEY:METHOD=AES-128,URI="/hls-key/key0.bin",IV=0Xcafe8f758ca555115584bb5b3c687f52\n#EXTINF:8,\n../../../../hls/streamNum82400.ts
\n#EXTINF:8,\n../../../../hls/streamNum82401.ts\n#EXT-X-KEY:METHOD=AES-128,URI="/hls-key/key.bin",IV=0X10ef8f758ca555115584bb5b3c687f52\n#EXTINF:8,\n../../../../hls/streamNum82400.ts\
n#EXTINF:8,\n../../../../hls/streamNum82401.ts\n#EXTINF:8,\n../../../../hls/streamNum82402.ts\n#EXTINF:8,\n../../../../hls/streamNum82403.ts\n#EXT-X-KEY:METHOD=AES-128,URI="/hls-key/key2.bin",IV=0Xcafe8f758ca555115584bb5b3c687f52\n#EXTINF:8,\n../../../../hls/streamNum82404.ts\n#EXTINF:8,\n../../../../hls/streamNum82405.ts'
E           #EXTM3U
E           #EXT-X-MEDIA-SEQUENCE:82400
E           #EXT-X-ALLOW-CACHE:NO
E           #EXT-X-VERSION:2
E           #EXT-X-TARGETDURATION:8
E         - #EXT-X-KEY:METHOD=AES-128,URI="/hls-key/key0.bin",IV=0Xcafe8f758ca555115584bb5b3c687f52
E           #EXTINF:8,
E           ../../../../hls/streamNum82400.ts
E           #EXTINF:8,
E           ../../../../hls/streamNum82401.ts
E           #EXT-X-KEY:METHOD=AES-128,URI="/hls-key/key.bin",IV=0X10ef8f758ca555115584bb5b3c687f52
E           #EXTINF:8,
E           ../../../../hls/streamNum82400.ts
E           #EXTINF:8,
E           ../../../../hls/streamNum82401.ts
E           #EXTINF:8,
E           ../../../../hls/streamNum82402.ts
E           #EXTINF:8,
E           ../../../../hls/streamNum82403.ts
E           #EXT-X-KEY:METHOD=AES-128,URI="/hls-key/key2.bin",IV=0Xcafe8f758ca555115584bb5b3c687f52
E           #EXTINF:8,
E           ../../../../hls/streamNum82404.ts
E           #EXTINF:8,
E           ../../../../hls/streamNum82405.ts

tests\test_model.py:840: AssertionError

This has actually failed, because segment.key is no long ever used when dumping/outputting the playlist. We instead use exclusively the keys list, I think overall this behavior/functionality should be removed, it's not going to be consistent and in this situation, it's not really fixable.

The best we could do is instead of using by_key(None), we could loop all segments and if segment.keys is empty, add it in. But this would mean by_key() is now even more useless. This also means any existing code using by_key for this would be broken.

I think overall, behavior relating to segment.by_key, especially by_key(None) seems broken. I'm not sure how you want to proceed from here. As I said the behavior of by_key is fundamentally flawed, and in a world where segments can have multiple keys, it falls flat.

We would need to decide to keep key and by_key for now, fixing the tests and adding some changes, or ditch both key and by_key, as well as the tests for them, then make new tests for keys.

This is because the segment may be constructed with both `key` and `keyobject` due to `Segment(..., **segment)` being done. Even though we do not use the `key` argument, we must define it.

This code assumes that the key carries forward to the next segments, which even before my changes I don't believe it would. Either way it definitely doesn't now, and this would be correct behaviour.

mauricioabreu · 2023-04-15T11:14:34Z

@rlaphoenix If I understood it right, your code is backwards compatible right? Maybe in a major update release we can remove the key and stick with the keys only

rlaphoenix · 2023-04-15T16:28:44Z

@rlaphoenix If I understood it right, your code is backwards compatible right? Maybe in a major update release we can remove the key and stick with the keys only

Correct. Pulling this as it is right now might not be the best idea. As stated before there's no real tests made for keys yet with some tests testing key still, and some modified to test keys. Some tests are failing like the by_key and stuff.

It's sort of up to you what you want to do. We could just drop key and the by_key() function, update the tests to work with keys. Or we could write new tests for keys and fix the remaining broken tests (e.g., by_key, which I feel should still go).

One thing I will say is that I use these current changes in another project (before the squashes/changes to add back key) and multiple people use it. It has been working like a charm with no problems requiring changes to this pull, and minimal changes to the projects that used key.

To be clear, the only issues right now seem to be fixing/adding tests to reflect the new keys. Though by_key is basically broken now. Though as stated before, by_key is fundamentally flawed and not useful anyway.

vevv · 2023-11-27T13:41:15Z

Is there anything still blocking this PR? This would be a very welcome update.

rlaphoenix · 2023-12-01T12:16:50Z

To be honest, there's been no advancements since the last comment I made. Basically, what was said in that comment still applies.

NikOverflow · 2024-10-14T15:20:45Z

what is happening with that?

mauricioabreu approved these changes Mar 19, 2023

View reviewed changes

Make sure Segment.keys is at least an empty list

7102383

rlaphoenix force-pushed the keep-preceeding-keys branch 6 times, most recently from 973ce64 to e5ed623 Compare March 26, 2023 09:23

rlaphoenix added 2 commits March 26, 2023 10:31

Update model.py

cb0f245

Re-implement Segment.key, having the last key within keys

69fa67e

This keeps compatibility and fixes a lot of the existing tests. However, new tests should likely be implemented for testing HLS playlists with multiple preceeding keys.

rlaphoenix force-pushed the keep-preceeding-keys branch from e5ed623 to 69fa67e Compare March 26, 2023 09:32

rlaphoenix added 3 commits March 26, 2023 10:40

Fix index mistake in two parser tests

36dbc84

Add key argument to Segment

fb266d0

This is because the segment may be constructed with both `key` and `keyobject` due to `Segment(..., **segment)` being done. Even though we do not use the `key` argument, we must define it.

Fix model test, test_segment_attribute_with_multiple_keys

dcadc0e

This code assumes that the key carries forward to the next segments, which even before my changes I don't believe it would. Either way it definitely doesn't now, and this would be correct behaviour.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework segment.key as segment.keys, a list of keys #313

Rework segment.key as segment.keys, a list of keys #313

rlaphoenix commented Mar 9, 2023 •

edited

Loading

mauricioabreu commented Mar 17, 2023

rlaphoenix commented Mar 17, 2023

mauricioabreu commented Mar 19, 2023

mauricioabreu commented Mar 26, 2023

rlaphoenix commented Mar 26, 2023 •

edited

Loading

mauricioabreu commented Apr 15, 2023

rlaphoenix commented Apr 15, 2023 •

edited

Loading

vevv commented Nov 27, 2023

rlaphoenix commented Dec 1, 2023

NikOverflow commented Oct 14, 2024

Rework segment.key as segment.keys, a list of keys #313

Are you sure you want to change the base?

Rework segment.key as segment.keys, a list of keys #313

Conversation

rlaphoenix commented Mar 9, 2023 • edited Loading

mauricioabreu commented Mar 17, 2023

rlaphoenix commented Mar 17, 2023

mauricioabreu commented Mar 19, 2023

mauricioabreu commented Mar 26, 2023

rlaphoenix commented Mar 26, 2023 • edited Loading

mauricioabreu commented Apr 15, 2023

rlaphoenix commented Apr 15, 2023 • edited Loading

vevv commented Nov 27, 2023

rlaphoenix commented Dec 1, 2023

NikOverflow commented Oct 14, 2024

rlaphoenix commented Mar 9, 2023 •

edited

Loading

rlaphoenix commented Mar 26, 2023 •

edited

Loading

rlaphoenix commented Apr 15, 2023 •

edited

Loading