support NAL unit overlapping between PES packets

[video-dev]

support NAL unit overlapping between PES packets

[mangui]

@tjenkinson indeed i can reproduce with 360p and 720p.
i checked 360p with flashls, and it is working fine, but from flashls debug logs I spotted the following:
DEBUG:AVC: no NALU slices found
this means that your stream has NAL unit overlapping between PES packets.
this case of NAL unit overlapping between PES packet is not handled yet in hls.js. this needs to be handled here : https://github.com/dailymotion/hls.js/blob/master/src/demux/tsdemuxer.js#L249
in flashls, when that happens, the overlapping unit is appended to the previously parsed one.

[tjenkinson]

Great thanks I might have a go at a fix later then.

Is this behaviour inline with the HLS spec?

On 22 Sep 2015, at 13:01, Guillaume du Pontavice [email protected] wrote:

@tjenkinson indeed i can reproduce with 360p and 720p.
i checked 360p with flashls, and it is working fine, but from flashls debug logs I spotted the following:
DEBUG:AVC: no NALU slices found
this means that your stream has NAL unit overlapping between PES packets.
this case of NAL unit overlapping between PES packet is not handled yet in hls.js. this needs to be handled here : https://github.com/dailymotion/hls.js/blob/master/src/demux/tsdemuxer.js#L249
in flashls, when that happens, the overlapping unit is appended to the previously parsed one.

—
Reply to this email directly or view it on GitHub.

[mangui]

HLS spec is quite open on the way TS fragments are packaged, as long as you have a PAT, a PMT, and a keyframe inside your fragments ...

[tjenkinson]

The comment here sounds like you already intended this to happen?
https://github.com/dailymotion/hls.js/blob/master/src/demux/tsdemuxer.js#L464

[mangui]

that one is a little bit different : it handles the case when a NALu has been found in the PES, but not starting at the beginning of the PES.
in your case, no NALu delimiter has been found in the entire PES packet.

[tjenkinson]

Ok I can see where this happens in the AS version (https://github.com/mangui/flashls/blob/dev/src/org/mangui/hls/demux/TSDemuxer.as#L426) but I'm having trouble trying to understand how it all works. It's the first time I've looked into a stream at packet level and there's a lot going on so it's going to take me while to figure that out first.

[mangui]

you need to follow same logic here:
https://github.com/dailymotion/hls.js/blob/master/src/demux/tsdemuxer.js#L249

// no NALu found
if(units.length === 0) {
append pes.data to previous NAL unit 
}

refer to https://github.com/dailymotion/hls.js/blob/master/src/demux/tsdemuxer.js#L469-L476 for NAL unit appending

[tjenkinson]

This looking right?

 _parseAVCPES(pes) {
    var units,track = this._avcTrack,avcSample,key = false;
    units = this._parseAVCNALu(pes.data);
    // no NALu found
    if(units.length === 0) {
      // append pes.data to previous NAL unit
      var lastavcSample = this._avcSamples[this._avcSamples.length-1];
      var lastUnit = lastavcSample.units.units[lastavcSample.units.units.length-1];
      var tmp = new Uint8Array(lastUnit.data.byteLength+pes.data.byteLength);
      tmp.set(lastUnit.data,0);
      tmp.set(pes.data,lastUnit.data.byteLength);
      lastUnit.data = tmp;
      lastavcSample.units.length+=pes.data.byteLength;
      this._avcSamplesLength+=pes.data.byteLength;
    }
    //free pes.data to save up some memory
    pes.data = null;
    units.units.forEach(unit => {

How does it know if it's not got a complete NAL unit though in the first packet?

[mangui]

if there is no NALu start delimiter, it is definitely not a complete NALu
LGTM
you also need to check that this._avcSamples.length > 0 to avoid RangeError

[tjenkinson]

So this is correct? I'm really confused

So this is the change?

 _parseAVCPES(pes) {
    var units,track = this._avcTrack,avcSample,key = false;
    units = this._parseAVCNALu(pes.data);
    // no NALu found
    if(units.length === 0 & this._avcSamples.length > 0) {
      // append pes.data to previous NAL unit
      var lastavcSample = this._avcSamples[this._avcSamples.length-1];
      var lastUnit = lastavcSample.units.units[lastavcSample.units.units.length-1];
      var tmp = new Uint8Array(lastUnit.data.byteLength+pes.data.byteLength);
      tmp.set(lastUnit.data,0);
      tmp.set(pes.data,lastUnit.data.byteLength);
      lastUnit.data = tmp;
      lastavcSample.units.length+=pes.data.byteLength;
      this._avcSamplesLength+=pes.data.byteLength;
    }
    //free pes.data to save up some memory
    pes.data = null;
    units.units.forEach(unit => {

I don't understand how the change this code makes effects the rest of this function, it doesn't appear to to me. I think I just need a more complete understanding.

I can make a pull request if you want?

[mangui]

this is it, you are almost there :-)
you also need to surround this code https://github.com/dailymotion/hls.js/blob/master/src/demux/tsdemuxer.js#L295-L298

by if(units.length) {

and in theory it should work

[tjenkinson]

I have tried to build it on my windows machine but I get quite a few errors when running npm run build, and also if I install browserify globally and run browserify -s hls src/hls.js -o dist/hls.js the hls.js file is created in 'dist', but when I load the demo page I get a console error saying Uncaught ReferenceError: Hls is not defined

The 'hls.js' script is getting loaded into the browser.

[tjenkinson]

This is the npm log when running npm run build with the current master branch.

0 info it worked if it ends with ok
1 verbose cli [ 'c:\\Program Files\\nodejs\\node.exe',
1 verbose cli   'c:\\Program Files\\nodejs\\node_modules\\npm\\bin\\npm-cli.js',
1 verbose cli   'run',
1 verbose cli   'build' ]
2 info using [email protected]
3 info using [email protected]
4 verbose run-script [ 'prebuild', 'build', 'postbuild' ]
5 info prebuild [email protected]
6 verbose unsafe-perm in lifecycle true
7 info [email protected] Failed to exec prebuild script
8 verbose stack Error: [email protected] prebuild: `npm run clean & npm run test`
8 verbose stack Exit status 1
8 verbose stack     at EventEmitter.<anonymous> (c:\Program Files\nodejs\node_modules\npm\lib\utils\lifecycle.js:213:16)
8 verbose stack     at EventEmitter.emit (events.js:110:17)
8 verbose stack     at ChildProcess.<anonymous> (c:\Program Files\nodejs\node_modules\npm\lib\utils\spawn.js:24:14)
8 verbose stack     at ChildProcess.emit (events.js:110:17)
8 verbose stack     at maybeClose (child_process.js:1015:16)
8 verbose stack     at Process.ChildProcess._handle.onexit (child_process.js:1087:5)
9 verbose pkgid [email protected]
10 verbose cwd i:\My Documents\GitHub\hls.js
11 error Windows_NT 6.3.9600
12 error argv "c:\\Program Files\\nodejs\\node.exe" "c:\\Program Files\\nodejs\\node_modules\\npm\\bin\\npm-cli.js" "run" "build"
13 error node v0.12.7
14 error npm  v2.11.3
15 error code ELIFECYCLE
16 error [email protected] prebuild: `npm run clean & npm run test`
16 error Exit status 1
17 error Failed at the [email protected] prebuild script 'npm run clean & npm run test'.
17 error This is most likely a problem with the hls.js package,
17 error not with npm itself.
17 error Tell the author that this fails on your system:
17 error     npm run clean & npm run test
17 error You can get their info via:
17 error     npm owner ls hls.js
17 error There is likely additional logging output above.
18 verbose exit [ 1, true ]

Shall I create a separate issue for this?

[mangui]

mmm we tested node on linux and OS X and it is working as expected.
this might be a windows specific issue or a node installation issue

[tjenkinson]

Thanks for building it I just tested locally and it does indeed work now :D

Browserify runs by itself, but for some reason there is no reference to Hls anywhere in the generated file, whereas in the one you build there is.

[mangui]

you might open a different ticket for node/win32 setup, I am closing this one as the ticket scope has been accomplished
thanks for the first public PR btw :-)