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

Stop TypeError when chunk.mediaType === fragmentedText #685

Merged

Conversation

davemevans
Copy link
Contributor

2ff50a0 in #674 did not fix the case where chunk is required to be passed to TextSourceBuffer.append - getIsTextTrack did not resolve correctly for fragmentedText and TypeError was thrown in append since chunk was not passed correctly.

I chose to fix this using hasOwnProperty to search for a property which should never appear on the native version. The other option I considered was using Function.length to determine the number of arguments append would accept. Both seem smelly, but it seems unlikely a native SourceBuffer will ever have a getTextTrackExtensions method.

This fix enables playback of http://rdmedia.bbc.co.uk/dash/ondemand/elephants_dream/1/client_manifest-all.mpd which used to work and was subsequently broken.

EDIT: checking for fragmentedText in getIsTextTrack isn't really an option since getIsTextTrack expects a mimeType and the mimeType is not know at this point. The ideal fix is not to vary the number of arguments to supposedly identical methods.

@davemevans
Copy link
Contributor Author

When I thought about it, what really needs to be detected is whether we are calling append on a native implementation or one we have created.

This patch looks at the prototype of the buffer. Our implementation (currently) has Object as its prototype. This should not be the case for native types which should have SourceBuffer or similar. I have tested this on FF, Edge, IE11, Chrome and Safari and found it to be working well.

In my opinion we really ought not to be attempting to handle varying function signatures in this code and should either refactor TextSourceBuffer.append so it does not need the second argument, or remove this attempt at making the native and ours interchangeable. This seems like the least worst solution in the short term, but feel free to comment.

@dsparacio
Copy link
Contributor

@bbcrddave some history here. when TextSourceBuffer was implemented it was assumed that some day according to the HTML spec that MSE Source buffer would be able to handle text tracks but was not the case when we needed it. So we "polyfill" with fake source buffer on the try catch error when we try to create a real source buffer but it fails due to the mimeType. Again assuming that when the browser catches up it will create an MSE source buffer for text and we bypass our implementation.

Are you saying that you tried to use the Native source buffer for text tracks in the other non chrome browsers and the native SB worked? Or just saying need a way to know if native or not AND that we should keep the same function signature as the native - which only takes one arg so our append should as well? I think that is how I am reading this. Have no real opinion on this yet but figured I would add this note about how we came to this solution .

@davemevans
Copy link
Contributor Author

Thanks for the history, Dan, and sorry for not being clearer.

What I was saying is that our polyfill is (IMO) no longer a polyfill since it requires special handling due to its requirement for a second argument. So it would be ideal if EITHER 1) it is refactored to become a true polyfill again OR 2) we just admit it isn't, in which case we need something similar to what I propose in the patch. (1) would be my preference.

The background to this PR is that Safari cannot accept a second argument to a native SB so we need to determine if the SB is native and not pass the second argument. The initial fix for #656 was not working correctly in all instances.

Finally, AFAICT, no UA currently provides native SB for TT (see seperate discussion in #690).

@dsparacio
Copy link
Contributor

@bbcrddave Thanks for the summary on your side. I fully agree, the second arg breaks the signature and the concept of pollfill. Just put some notes in the line note you added to the multi track PR... I will look at all the notes and pull in the related PRs and offer a solution.

dsparacio pushed a commit that referenced this pull request Aug 6, 2015
Stop TypeError when chunk.mediaType === fragmentedText
@dsparacio dsparacio merged commit 91799f3 into Dash-Industry-Forum:development Aug 6, 2015
@davemevans davemevans deleted the FixFragmentedTextAppend branch August 7, 2015 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants