membraneframework / membrane_core

The core of the Membrane Framework, advanced multimedia processing framework
https://membrane.stream
Apache License 2.0
1.22k stars 35 forks source link

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

Closed jerzywilczek closed 4 months ago

jerzywilczek commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

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

mat-hek commented 4 months ago

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

Qizot commented 4 months ago

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

mat-hek commented 4 months ago

@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 commented 4 months ago

@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 commented 4 months ago

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

mat-hek commented 4 months ago

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