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

Changed MjpegInputStreamDefault to handle IllegalArgumentException in parseContentLength #27

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lorenzos
Copy link
Contributor

@lorenzos lorenzos commented Mar 5, 2017

I copied some code from the original SimpleMjpegView's MjpegInputStream into your MjpegInputStreamDefault implementation.

The reason for this is because I got many IllegalArgumentException when parsing frames' content length using your library. When I tested the original SimpleMjpegView these errors went gone, and I noticed it is because the exception is caught and content length is retrieved using a different approach (searching for the end of the frame instead).

The reason I'm not just using SimpleMjpegView is because I have some trouble in decoding JPEG frames using its native code, often I got glitched images from my D-Link security camera. Instead, your default implementation, that uses BitmapFactory.decodeStream, always works well.

One last note. I don't know why your input stream implementation was different from the original library. I didn't manage to understand if you purposely changed it (in that case I somehow reverted it), and if yes, why you did it. Or if you copied it and later the original library was modified to handle errors similar to mine (in that case I ported those changes here). Please let me know!

@niqdev
Copy link
Owner

niqdev commented Mar 6, 2017

Hi,
to be honest I think the comment on the class is self explanatory 😅
I don't really remember if there was a reason and I would like to ask you before merge the PR if you can please confirm that your changes don't introduce any regression in the sample app and others no-D-Link ipcam.
Thanks

@lorenzos
Copy link
Contributor Author

lorenzos commented Mar 7, 2017

Ok, now that you made me pay more attention to that comment of yours, I think I understood the situation. Both of you copied that class from Google's Axis camera repo (yours is identical before my PR), and then SimpleMjpegView fixed some stuff independently. So I'm pretty sure what I'm doing here is porting some SimpleMjpegView's fixes into your library.

Unfortunately I've got no other IP-cam MJPEG stream to test. The only one, aside from my camera, is this one I randomly found here, and I can confirm it works. If you are aware of any other public stream, I'm glad to test all of them.

Maybe you can make your own test with your camera(s) using JitPack to include my fork's branch in Android Studio (that's what I'm doing in my project right now):

// In project level gradle
allprojects {
	repositories {
		jcenter()
		maven { url "https://jitpack.io" }
	}
}

// In app's module gradle
dependencies {
	// compile 'com.github.niqdev:mjpeg-view:0.5.0'
	compile 'com.github.lorenzos:ipcam-view:reverted-default-input-stream-SNAPSHOT'
}

@lorenzos
Copy link
Contributor Author

lorenzos commented Mar 7, 2017

Maybe @neuralassembly himself can help us understanding the exact reason for his fixes, and if they can break compatibility with any MJPEG stream 😄

@niqdev
Copy link
Owner

niqdev commented Mar 7, 2017

Yep,
you could simply run the app example in the project, in particular see this activity. If that works I think is fine.
I would by glad if the authors of the libs would help us, I don't understand how the underlying implementation works and never had enough time to debug it.

@niqdev
Copy link
Owner

niqdev commented Mar 7, 2017

By the way the aim of the library is to wrap 2 different implementation:

the difference between the 2 is that the second one uses the native C code (I'm expecting better performance), but cos I had too many issues already with the first I didn't fully integrate the native one

@niqdev
Copy link
Owner

niqdev commented Mar 12, 2017

@lorenzos did you manage to use the native version?
Thanks

@lorenzos
Copy link
Contributor Author

@niqdev Nope, sorry for the waiting. I'll test my PR with your example project as soon as possible.

@niqdev
Copy link
Owner

niqdev commented Mar 12, 2017

No rush, just compare the 2 implementation, I think you are interested in the native one

@niqdev
Copy link
Owner

niqdev commented Dec 2, 2018

Hi @lorenzos, I'm doing some cleanup.
Is there any progress on this PR or I shuld just close it and ignore it?
Thanks

@lorenzos
Copy link
Contributor Author

lorenzos commented Dec 3, 2018

Nope. To be honest, I don't remember if I made the tests we were talking about. I'm still using my fix in my app, but I don't feel I can tell if you should merge or not. Fell free to close the PR.

@niqdev
Copy link
Owner

niqdev commented Dec 3, 2018

Thanks @lorenzos and don't worry, I'll wait for a feedback from @jcRuan related to #70 before close it

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