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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 7 commits into from

Conversation

rlaphoenix
Copy link

@rlaphoenix rlaphoenix commented Mar 9, 2023 •
edited

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.

@rlaphoenix
Rework segment.key as segment.keys, a list of keys
e58e878 
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
Copy link
Member

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

@rlaphoenix
Copy link
Author

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
Copy link
Member

@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
Copy link
Member

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

@rlaphoenix rlaphoenix force-pushed the keep-preceeding-keys branch 6 times, most recently from 973ce64 to e5ed623 Compare March 26, 2023 09:23
@rlaphoenix
Update model.py
cb0f245
@rlaphoenix
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
Copy link
Author

rlaphoenix commented Mar 26, 2023 •
edited

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.

@rlaphoenix
Fix index mistake in two parser tests
36dbc84
@rlaphoenix
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.
@rlaphoenix
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.
@mauricioabreu
Copy link
Member

@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
Copy link
Author

rlaphoenix commented Apr 15, 2023 •
edited

@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
Copy link
Contributor

vevv commented Nov 27, 2023

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

@rlaphoenix
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mauricioabreu mauricioabreu mauricioabreu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Parsing and dumping of multiple EXT-X-KEYs per segment is not supported
3 participants
@rlaphoenix @mauricioabreu @vevv