-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: add container requesting and checking logic #12
Conversation
src/byte-helpers.js
Outdated
try { | ||
return decodeURIComponent(escape(string)); | ||
} catch (e) { | ||
// if decodeURIComponent/escape fails, we are dealing with non string data, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is non string data, and the data should be returned, should bytes be returned, or empty string? What will string
be in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case string will usually be a garbled mess of characters, perhaps we should return empty string in that case, but that could be problematic because it would make using this function a bit harder in some cases.
Example 1: Where returning a partially garbled string may help
You think the string ID3
starts a byte 0 and ends at byte 4, while really it ends at byte 3.
You call this function with a subarray of 0 to 4. string
will be ID3~
where tilde is a garbled character. if we returned an empty string here you wouldn't really know what is going on.
Example 2: Where returning a partially garbled string doesn't help, but doesn't really hurt either.
You think the string 竜竜
starts a byte 0 and ends at byte 5, while really it ends at byte 4 (since non-ascii characters codes are greater than 255).
You call this function with a subarray of 0 to 5. string
will be entirely garbled ~#~#)
. Regardless of what we return it is confusing, but even the garbled charactes could help you determine that the character codes are off as ~#
in google may lead you to someone who tells you that that is actually character 387 which is 竜
and from there you know your off by one byte.
src/containers.js
Outdated
}, | ||
|
||
ts(bytes) { | ||
return (bytes.length >= 189 && bytes[0] === 0x47 && bytes[188] === 0x47) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, mux.js accounts for misaligned ts streams: https://github.com/videojs/mux.js/blob/master/lib/m2ts/m2ts.js#L69
Should we also try to account for them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it some more I think that it shouldn't really be valid or we can't handle it. I think mux.js does that because it actually handles full segments and sometimes we lose which byte we are on somehow (edited)
4:37
I think the user of isLikely.ts needs to make sure that they are passing in the first 188 bytes of a ts segment for it to work correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the package-lock.json
managed to get a lot of changes (with the package.json
change removed).
reverted the package-lock changes. now it only has the removal of sinon which we don't use. |
dcc810b
to
6b9b10b
Compare
src/containers.js
Outdated
}; | ||
|
||
// fmp4 is not a container | ||
export const isLikelyFmp4 = (bytes) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it was just checking for 'moof' before, but since we do, maybe we should rename to something like isLikelyFmp4MediaSegment (since moof refers to media segments: https://www.w3.org/TR/mse-byte-stream-format-isobmff/#iso-media-segments ).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed
QUnit.module('isLikelyFmp4'); | ||
QUnit.test('works as expected', function(assert) { | ||
const fmp4Data = [] | ||
.concat(createBox('styp')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since styp
is optional, probably worth adding a test without it too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
In VHS we are running into codec issues now that we are more permissive with codecs that the browser supports during fmp4 playback. This causes problems for sidx content and sidx content can contain webm segments, and while the browser supports it, we cannot parse it. This code will also be useful for doing minimal requests to get the codec for a segment in 10-12 bytes rather than an entire segment.