GNOME Bugzilla – Bug 766301
qtdemux: Parsing elst box based on version
Last modified: 2017-01-31 15:10:48 UTC
qtdemux: Parsing elst box based on version segment_duration and media_time should be parsed based on version of elst box. Specification defines that an elst box with version 1 has uint64 and int64 values for segment_duration and media_time, respectively.
Created attachment 327672 [details] [review] qtdemux: Parsing elst box based on version
- Current qtdemux does not consider about version of elst box. It causes incorrect SEGMENT event (i.e., abnormal segment.start value) - Following is the specification of elst section. 8.6.6.2 Syntax aligned(8) class EditListBox extends FullBox(‘elst’, version, 0) { unsigned int(32) entry_count; for (i=1; i <= entry_count; i++) { if (version==1) { unsigned int(64) segment_duration; int(64) media_time; } else { // version==0 unsigned int(32) segment_duration; int(32) media_time; } int(16) media_rate_integer; int(16) media_rate_fraction = 0; } } Log messages are as follows <NG case> 388 0:00:00.087622976 ^[[336m 5195^[[00m 0x7fdc140f1de0 ^[[37mDEBUG ^[[00m ^[[00m qtdemux qtdemux.c:8420:qtdemux_parse_segments:<qtdemux0>^[[00m looking for edit list container 389 0:00:00.087626987 ^[[336m 5195^[[00m 0x7fdc140f1de0 ^[[37mDEBUG ^[[00m ^[[00m qtdemux qtdemux.c:8431:qtdemux_parse_segments:<qtdemux0>^[[00m looking for edit list 390 0:00:00.087631099 ^[[336m 5195^[[00m 0x7fdc140f1de0 ^[[33;01mWARN ^[[00m ^[[00m qtdemux qtdemux.c:8497:qtdemux_parse_segments:<qtdemux0>^[[00m found suspicious rate 0 391 0:00:00.087635509 ^[[336m 5195^[[00m 0x7fdc140f1de0 ^[[37mDEBUG ^[[00m ^[[00m qtdemux qtdemux.c:8512:qtdemux_parse_segments:<qtdemux0>^[[00m created segment 0 time 0:00:00.000000000, duration 99:99:99.999999999, media_start 0:02:44.415000000 (3945960) , media_stop 0:02:44.414999999 stop_time 0:02:44.414250000 rate 1, (0) timescale 24000 <OK case with this patch> 388 0:00:00.028864836 ^[[333m 3782^[[00m 0x7f8e3410f1e0 ^[[37mDEBUG ^[[00m ^[[00m qtdemux qtdemux.c:8420:qtdemux_parse_segments:<qtdemux0>^[[00m looking for edit list container 389 0:00:00.028868896 ^[[333m 3782^[[00m 0x7f8e3410f1e0 ^[[37mDEBUG ^[[00m ^[[00m qtdemux qtdemux.c:8432:qtdemux_parse_segments:<qtdemux0>^[[00m looking for edit list 390 0:00:00.028873235 ^[[333m 3782^[[00m 0x7f8e3410f1e0 ^[[37mDEBUG ^[[00m ^[[00m qtdemux qtdemux.c:8526:qtdemux_parse_segments:<qtdemux0>^[[00m created segment 0 time 0:00:00.000000000, duration 0:02:44.415000000, media_start 0:00:00.125125000 (3003) , media_stop 0:02:44.540125000 stop_time 0:02:44.415000000 rate 1, (65536) timescale 24000 391 0:00:00.028883972 ^[[333m 3782^[[00m 0x7f8e3410f1e0 ^[[33;01mWARN ^[[00m ^[[00m qtdemux qtdemux.c:8532:qtdemux_parse_segments:<qtdemux0>^[[00m Segment 0 extends to 0:02:44.415000000 past the end of the file duration 0:02:44.414250000 it will be truncated
A unit test would be really nice, looks good otherwise.
Comment on attachment 327672 [details] [review] qtdemux: Parsing elst box based on version Agreed. A unit test would be nice, but considering we have none at al for qtdemux yet it would make more sense to start with something that tests the basics and then add new things on top of that. But can you provide a sample file with this behaviour please?
(In reply to Sebastian Dröge (slomo) from comment #4) > Agreed. A unit test would be nice, but considering we have none at al for > qtdemux yet it would make more sense to start with something that tests the > basics and then add new things on top of that. -good/tests/check/elements/qtmux is for both muxer and demuxer. We do add tests there these days. I even wrote one.
But that's more a test of the muxer, or the demuxer/muxer combination. Having individual qtdemux tests would also be good, and in this case having a demuxer/muxer test would also require to add a feature to the muxer (to write the other version of this box).
(Also thanks, I didn't realize we have tests there using qtdemux)
(In reply to Sebastian Dröge (slomo) from comment #4) > Comment on attachment 327672 [details] [review] [review] > qtdemux: Parsing elst box based on version > > Agreed. A unit test would be nice, but considering we have none at al for > qtdemux yet it would make more sense to start with something that tests the > basics and then add new things on top of that. > > But can you provide a sample file with this behaviour please? I really want to share the file but, unfortunately, it cannot be shared due to policy... In this context, I think unit test code with adding a feature to muxer is an alternative for verification.
Comment on attachment 327672 [details] [review] qtdemux: Parsing elst box based on version Attachment 327672 [details] pushed as 56e273b - qtdemux: Parsing elst box based on version
Seungha Yang, are you preparing a patch for the test? The fix itself is going to be in 1.8.2 and 1.9.1
Sorry, I was busy for other works. To adding new feature in qtmux, I'm looking the mux code, but frankly speaking, I've never worked on mux so need time... I'm preparing it and will report test code with new feature in mux.
(In reply to Sebastian Dröge (slomo) from comment #10) > Seungha Yang, are you preparing a patch for the test? The fix itself is > going to be in 1.8.2 and 1.9.1 I upload a patch for qtmux (bug #766760). However, that patch is just to modify variables from 32bit to 64bit (elst). When I saw qtmux code, version of most atoms are fixed to "version 0"... So, I think that current qtmux does not consider overflow of duration. Not only elst, but other boxes such as mvhd, mdhd, and trak can have 64bit and in order to use it, the version should be "version 1". What I'm going to do is - 1. Apply patch bug #766760 which can give a chance to support "version 1" elst. (but muxing using "version 1" boxes is not implemented yet in qtmux) - 2. Adding another feature in qtmux to support "version 1" boxes based on checked duration (whether it overs G_MAX_UINT32 or not), or new properties... with test code.
(In reply to Seungha Yang from comment #12) > (In reply to Sebastian Dröge (slomo) from comment #10) > > Seungha Yang, are you preparing a patch for the test? The fix itself is > > going to be in 1.8.2 and 1.9.1 > > What I'm going to do is > - 1. Apply patch bug #766760 which can give a chance to support "version 1" > elst. (but muxing using "version 1" boxes is not implemented yet in qtmux) > - 2. Adding another feature in qtmux to support "version 1" boxes based on > checked duration (whether it overs G_MAX_UINT32 or not), or new > properties... with test code. Since the only thing missing in this bug report is the unit test, that can be handled in the other bug report. Closing.