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

[MP4] MP4 plugin has a test case with audio and video but the HLS playlist only lists video. #759

Closed
jerzywilczek opened this issue Feb 20, 2024 · 9 comments

Comments

@jerzywilczek
Copy link

jerzywilczek commented Feb 20, 2024

https://github.com/membraneframework/membrane_mp4_plugin/blob/8b55570faaecfa79b1eeffb4b8077b18624f1f25/test/fixtures/cmaf/muxed_audio_video/index.m3u8#L4C44-L4C48

Other than the issue mentioned in the topic, some values in the mp4 seem to be strange, e.g. the second sidx box in the first segment has earliest_presentation_time = 18446744073709537000, which does not seem right...

@mat-hek
Copy link
Member

mat-hek commented Feb 21, 2024

https://github.com/membraneframework/membrane_mp4_plugin/blob/8b55570faaecfa79b1eeffb4b8077b18624f1f25/test/fixtures/cmaf/muxed_audio_video/index.m3u8#L4C44-L4C48

It's more like an HLS issue, @Karolk99 why don't we put the audio codec in there as well?

the second sidx box in the first segment has earliest_presentation_time = 18446744073709537000

Indeed, @Qizot any idea why is that?

some values in the mp4 seem to be strange

@jerzywilczek any others that feel strange?

@Qizot
Copy link
Member

Qizot commented Feb 21, 2024

@mat-hek earliest_presentation_time in the schema is represented by :uint64 where I believe it should be :int64?

If you change the given time to a signed integer you get -14616.

@jerzywilczek
Copy link
Author

@mat-hek I think other timestamps may be wrong too, don't remember anymore

@mat-hek
Copy link
Member

mat-hek commented Feb 21, 2024

@Qizot it should be unsigned according to the spec. It seems we pass a negative value there and we shouldn't.

@Qizot
Copy link
Member

Qizot commented Feb 21, 2024

@mat-hek This fragment takes DTS instead of the PTS value so here is the bug.

@mat-hek
Copy link
Member

mat-hek commented Feb 22, 2024

@Qizot it fixes the problem, but unfortunately it's also somehow used for calculating durations. Changing it makes the following tests fail:

  3) test video with partial segments should create as many partial segments as possible until reaching a key frame (Membrane.MP4.Muxer.CMAF.IntegrationTest)
     test/membrane_mp4/muxer/cmaf/integration_test.exs:215
     Assertion with <= failed
     code:  assert buffer.metadata.duration <= Membrane.Time.milliseconds(550)
     left:  1000000000
     right: 550000000
     stacktrace:
       test/membrane_mp4/muxer/cmaf/integration_test.exs:227: (test)
  6) test video partial segments (Membrane.MP4.Muxer.CMAF.IntegrationTest)
     test/membrane_mp4/muxer/cmaf/integration_test.exs:170
     Assertion with < failed
     code:  assert buffer.metadata.duration < Membrane.Time.milliseconds(550)
     left:  1000000000
     right: 550000000
     stacktrace:
       test/membrane_mp4/muxer/cmaf/integration_test.exs:182: anonymous fn/3 in Membrane.MP4.Muxer.CMAF.IntegrationTest."test video partial segments"/1
       (elixir 1.15.4) lib/enum.ex:4379: Enum.reduce/3
       test/membrane_mp4/muxer/cmaf/integration_test.exs:179: (test)

@Karolk99
Copy link
Contributor

@mat-hek, the link you referred to for the master manifest in the mp4_plugin is a hardcoded fixture from three years ago. If you look at the fixtures in the HTTP Adaptive Stream Plugin (for example, master manifest), you'll see that they include the audio codec.

@mat-hek
Copy link
Member

mat-hek commented Feb 26, 2024

Ok, so we just don't compare manifests as we focus on the m4s files, thus the outdated fixtures. I'm removing them in membraneframework/membrane_mp4_plugin#103. @jerzywilczek have a look at the HTTP adaptive stream plugin for manifest-related stuff.

@mat-hek
Copy link
Member

mat-hek commented Feb 26, 2024

Closing as membraneframework/membrane_mp4_plugin#103. and membraneframework/membrane_mp4_plugin#104 are merged, @jerzywilczek feel free to report more weird values :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

4 participants