Zms/videostream improvements #404

Sune1337 · 2014-04-25T21:01:41Z

Support streaming with zms (using ffmpeg) better …

* ZoneMinder compiles with ffmpeg 0.5..master
* Make VideoStream class able to stream through ffmpeg better.
* Ability to use fixed quality instead of fixed bitrate (by specifying bitrate 0...100)
* Format url parameter supports <format>/<encoder> syntax

See commit message for details.

* ZoneMinder compiles with ffmpeg 0.5..master * Make VideoStream class able to stream through ffmpeg better. * Ability to use fixed quality instead of fixed bitrate (by specifying bitrate 0...100) * Format url parameter supports <format>/<encoder> syntax Details: - Redefine av_err2str to a to avoid compiler warning on newer g++'s - When using rtp format; if ffmpeg does not have a default codec, use CODEC_ID_MPEG4. (because this is what the default is as of 2.2 when this code was written) - Specify ofc->packet_size if rtp format is requested - Video generated in a thread to guarantee a constant fps. - Move _AVCODECID definition into zm_ffmpeg.h (and use instead of (AV)CodecID - Call avformat_network_init because ffmpeg warns about it beeing required soon. - increase VideoStream::video_outbuf_size to 4MiB to be able to encode larger pictures

- fixed compilation issue with sws_..isSupported when compiling with ffmpeg 0.5

Sune1337 · 2014-04-25T21:05:55Z

This is the cleaned up version of #382 as agreed!

Sune1337 · 2014-04-25T21:08:54Z

A good format to use is "&format=mpegts/mpeg4". It works pretty long way back with FFmpeg.
Personally i cannot use the ZoneMinder default "mpeg" since my FFmpeg can't encode 5FPS using that.

Otherwise the point of this request is to give the user (me :)) better control for his/hers own purposes.

Sune1337 · 2014-04-25T21:12:57Z

The two last commits (4767326 and 3155d62) is "bugfixes" to the first commits. Consider it part of the first commit.

this is to deliver frames more accurately to the client

mastertheknife · 2014-04-28T14:33:38Z

src/zm_mpeg.cpp

-		uint8_t *opicture_buf = (uint8_t *)av_malloc(size);
+
+		int size = avpicture_get_size( c->pix_fmt, c->width, c->height );
+		uint8_t *opicture_buf = (uint8_t *)av_malloc( size );


Shouldn't av_mallocz be used here?
Oops, looks not, my bad.

Sune1337 · 2014-04-28T15:17:11Z

When you say it like that it sounds so obvious :)

knight-of-ni · 2014-04-29T14:53:58Z

@Sune1337 he does that to me too :-)

Not sure if you know this, but we are usually online on the #zoneminder freenode irc channel. You are welcome to join us.

knight-of-ni · 2014-10-10T12:39:44Z

Unless I hear otherwise from someonese else, I am going to give this a shot at merging this weekend.

It can't be auto-merged so I'll have to manually address whatever merge conflicts exists.

When it comes to troubleshooting zoneminder's streaming video C code, my understanding of what is going on is very limited. Consequently, @Sune1337 if you have a second and can take a look at what the problem might be, you are more likely to be able to fix the issue much faster than I can.

Anyway, I'll give it a shot.

Sune1337 · 2014-10-10T15:52:02Z

I had some uncommitted code in this branch; im gonna investigate what that is tomow. I'll also check out the conflicts.

Sune1337 · 2014-10-11T16:46:30Z

My uncommitted changes is an attempt to implement variable framerate; which failed at the time. those are not important for this pull-request, so i'll just keep that experiment locally.

I'll check out the conflicts now.

Sune1337 · 2014-10-11T17:28:46Z

I have resolved the conflicts.

as in #351 i will await confirmation that checking in alot of files in this branch will actually help you merge, and not make thins messier :)

To repeat: If i commit now i'll commit changes to a lot of files. I guess these are the changes between this branch and the ZoneMinder-master. Is this correct? Should i do the commit and push?

Sune1337 · 2014-10-11T18:19:55Z

I'll paste files to help you resolve conflicts on this pull request too.

What i did:
git clone https://github.com/ZoneMinder/ZoneMinder -b master ZoneMinder-master
git remote add upstream https://github.com/Sune1337/ZoneMinder -t zms/videostream-improvements
git fetch upstream
git merge upstream/zms/videostream-improvements
git mergetool

Resolves conflicts:
src/zm_ffmpeg.h: http://pastebin.com/N1r19hTH
src/zm_mpeg.cpp: http://pastebin.com/kRjgkZDp

edit: the zm_ffmpeg.h pastebin had been deleted somehow. updated the link.

knight-of-ni · 2014-10-11T20:18:36Z

This cannot be auto-merged due to conflicts with the following pull request that has already been merged: #325
I suspected that was the pull request that caused this.

There appear to be just two conflicts, and we need to figure out how to merge your changes in w/o unravelling the changes from that pull request.

For the conflict in zm_mpeg.cpp, you are using _AVCODECID_NONE while the author of #325 chose to use AV_CODEC_ID_NONE. Another example uses "of->video_codec" while you are using "codec_id". I don't know exactly what the end results should be.... probably a merge of your changes and his, but that's all I know right now.

Can you compare what is going on in #325 and then tell me how this new code should look?
You may want to consider making a new pull request.

Sune1337 · 2014-10-11T20:27:31Z

I also saw #325. I have accounted for this in the pastebin files (I use the AV_CODEC_ID_NONE as the author of #325 does).

Regarding the use of video_codec vs codec_id i'm not sure what you mean. Do you mean that i use CODEC_ID_* macros instead of AV_CODEC_ID_* ?

knight-of-ni · 2014-10-11T20:32:23Z

No, it means I just don't know what I am looking at and didn't understand which to use.

Sune1337 · 2014-10-11T20:33:22Z

I gave you 2 pastebin files above which i intended for you to use when resolving the conflicts. Isn't it the zm_ffmpeg.h and zm_mpeg.cpp that conflicts?

knight-of-ni · 2014-10-11T20:36:13Z

I'm just trying to understand it first. Give me a minute.

knight-of-ni · 2014-10-11T20:45:44Z

Arrrghh - this doesn't build.

/home/abauer/rpmbuild/BUILD/ZoneMinder-1.27.99/src/zm_ffmpeg.h:48:83: error: missing binary operator before token "("
 #if defined(HAVE_LIBAVCODEC_AVCODEC_H) && LIBAVCODEC_VERSION_INT >= AV_VERSION_INT(54,25,0)
                                                                                   ^
make[2]: *** [src/CMakeFiles/zm.dir/zm_curl_camera.cpp.o] Error 1

libavutil 52. 48.101 / 52. 48.101
libavcodec 55. 39.101 / 55. 39.101
libavformat 55. 19.104 / 55. 19.104
libavdevice 55. 5.100 / 55. 5.100
libavfilter 3. 90.100 / 3. 90.100
libavresample 1. 1. 0 / 1. 1. 0
libswscale 2. 5.101 / 2. 5.101
libswresample 0. 17.104 / 0. 17.104
libpostproc 52. 3.100 / 52. 3.100

knight-of-ni · 2014-10-11T21:07:34Z

This does not build on Fedora 20 w/ ffmpeg 2.1.1 (error is in previous post)
This builds on CentOS 6 w/ ffmpeg 0.10.15
Travis built this successfully on Ubuntu w/ ffmpeg 2.4.1

knight-of-ni · 2014-10-11T21:28:51Z

Further progress... forgot that my build environment for Fedora tries to build w/o ffmpeg by default.
If I enable ffmpeg support, it builds fine. What is broken is the ability to build zoneminder w/o any ffmpeg support... that explains why it could not find AV_VERSION_INT.

Sune1337 · 2014-10-11T21:32:32Z

Do you want me to create a pull request that can compile w/o ffmpeg?

Sune1337 · 2014-10-15T12:05:00Z

it seems the avformat_alloc_output_context2 isn't the only function not supported by libav (the fork).
Next issue is av_opt_set_defaults, then avcodec_get_name. then i didn't look more..

Sune1337 · 2014-10-15T12:06:05Z

I don't like libav now.

Sune1337 · 2014-10-15T14:13:58Z

I created https://github.com/Sune1337/ZoneMinder_dup/tree/libav_the_fork_compat which compiles iwth libav. ZMS do however coredump if one is trying to stream using libav video.

knight-of-ni · 2014-10-15T14:27:45Z

I wonder if we should just revert back to the old code path if building against libav. We know that worked, right? We could always come back to this and improve it after we release 1.28.0.

Sune1337 · 2014-10-15T14:29:55Z

the coredump was a bug; should exist when compiling with ffmpeg too.

Sune1337 · 2014-10-15T14:31:08Z

ps. i never got the zms to stream video before this pullrequest. ever. and i never saw anyone claiming they did either,, so i don't agree with old code working.

Sune1337 · 2014-10-15T14:32:11Z

i could test-stream now with libav using AVI format. seems there are some differences in mpegts handling (which was the other format i tested)

Sune1337 · 2014-10-15T14:37:58Z

do you want me to create pull-request on the branch? If nothing else, to suggest a solution to compiling with libav.

knight-of-ni · 2014-10-15T14:41:48Z

Yeah, and I'll ask @connortechnology to test. I don't have an environment to test libav from so I'm going to be relying on others to tell what works and what doesn't.

Also, can you make line 61 do this?

#if ((LIBAVFORMAT_VERSION_INT >= AV_VERSION_INT(53, 5, 0)) && (LIBAVFORMAT_VERSION_MICRO >= 100))

I did like the extra info in those debug statements from your original pull request. Maybe we can get those back in sometime later after we develop a compatible solution.

Sune1337 · 2014-10-15T14:49:39Z

Do you mean change zm_ffmpege.cpp:61 from:

    AVFormatContext *s= avformat_alloc_context();

into

#if ((LIBAVFORMAT_VERSION_INT >= AV_VERSION_INT(53, 5, 0)) && (LIBAVFORMAT_VERSION_MICRO >= 100))
    avformat_alloc_output_context2( &ofc, NULL, format, filename );
#else
    AVFormatContext *s= avformat_alloc_context();

Sune1337 · 2014-10-15T14:57:59Z

btw; can you use 'LIBAVFORMAT_VERSION_MICRO >= 100' to determine if we're using libav or ffmpeg?

knight-of-ni · 2014-10-15T14:58:09Z

Yes.

I was looking at zm_ffmpeg.cpp from your branch:
https://github.com/Sune1337/ZoneMinder_dup/blob/master/src/zm_mpeg.cpp#L61

We want that condition to be false if using libav, but I'm assuming we are looking for a solution that uses avformat_alloc_output_context2 but only when building against ffmpeg.

knight-of-ni · 2014-10-15T14:59:51Z

You can use LIBAVFORMAT_VERSION_MICRO >= 100 but it may not be that simple depending on what you are trying to do.

Sune1337 · 2014-10-15T14:59:52Z

oh, the changes is in a branch called 'libav_the_fork_compat'.

But i'm gonna use the 'LIBAVFORMAT_VERSION_MICRO >= 100' method instead as you suggested. I agree this is better.

knight-of-ni · 2014-10-15T15:01:50Z

@SteveGilvarry posted a good example from libvlc earlier, but I think in our particular case we can just tack on LIBAVFORMAT_VERSION_MICRO.

In time if we run into more of these, then we may have to look into doing something more encompasing like in libvlc library.

Sune1337 · 2014-10-15T15:26:58Z

So; i have started to get stuff sorted out :)
I'll delete the branch i created above and create a new one to get stuff fixed the correct way.

Sune1337 · 2014-10-15T16:09:28Z

Man, github is messing with me.

To be able to create a branch for the fixes, i needed to fork the ZoneMinder repo again. I needed to do this since i cant remove my existing one. I cant use my existing one because i have an active pull-request on that. Phew.

github.com does not allow one to create a second fork, so i had to do this manually.

Now i cant create pull-requests on the new fork; github.com isnt aware of the "connection".

Can you use https://github.com/Sune1337/ZoneMinder_dup/tree/libav_the_fork_compat and create some manual pull request?

https://github.com/Sune1337/ZoneMinder_dup/tree/libav_the_fork_compat now contains what i think is the correct way of adressing the issues caused by #404 in regards to libav the fork.

Sune1337 · 2014-10-15T16:51:51Z

i just pushed a new commit "compile with ffmpeg <= 0.8". Found issues with older FFmpegs.

knight-of-ni · 2014-10-15T17:03:06Z

Yeah, that's why you don't create pull requests against your own master branch.

When viewing a commit, one can add ".patch" to the url and github will auto-create a patch.
If you can identify exactly which commits need to be applied, then maybe we can get @connortechnology to patch his source and test.

Sune1337 · 2014-10-15T17:08:08Z

It's all the commits in the branch "libav_the_fork_compat"; here are direct links to all patch files.

https://github.com/Sune1337/ZoneMinder_dup/commit/6e77632a5946097bcb5489dce999814f1110716b.patch
https://github.com/Sune1337/ZoneMinder_dup/commit/4b47336778aa76050aa6110d0420de8413586195.patch
https://github.com/Sune1337/ZoneMinder_dup/commit/df4a9f27751592655f2e1a20e58079c7d2985fe2.patch
https://github.com/Sune1337/ZoneMinder_dup/commit/055506838d40834d104c60daf8e5f264828cb050.patch

Sune1337 · 2014-10-15T17:11:07Z

The pull request on master branch is an old mistake i'll never forget. Even so, i guess there's nothing i can do about it as it's already done.

knight-of-ni · 2014-10-15T17:23:00Z

Sometimes learning the hard way is a good way to learn. I've made many github mistakes and have learned from it. honestly, I have learned to really like github.
As far as I know the only way to resolve the issue is to either close that open pull request or merge it... but I think we are going to hold that off until after 1.28.0 is released

Sune1337 · 2014-10-15T17:24:30Z

the commit messages for easy reviwing:

6e77632

use AV_CODEC_ID_MPEG4 instead of CODEC_ID_MPEG4

4b47336

fix compilation issues with libav: …
Sune1337 authored an hour ago
use AV_CODEC_ID_MPEG4 instead of CODEC_ID_MPEG4
added missing #include <libavutil/opt.h>
fixed with "LIBAVFORMAT_VERSION_MICRO >= 100":
libav does not support avformat_alloc_output_context2.
libav does not support avcodec_get_name.

df4a9f2

bugfix; coredump if no codec name was specified in url.

0555068

compile with ffmpeg <= 0.8

knight-of-ni · 2014-10-15T17:26:17Z

@connortechnology is building it now. Sounds like it worked. He is just going to merge the changes directly into master.

Sune1337 · 2014-10-15T17:26:42Z

sounds great

Sune1337 · 2014-10-15T18:18:33Z

I checked out the commits; the commit "bugfix; coredump if no codec name was specified in url. " is missing from the master branch. Its not really related to compilation issues; just a bugfix i sneaked in there :) just wanted to point it out.

connortechnology · 2014-10-15T18:35:39Z

I've got it now. Now sure how I missed it. Thanks for your efforts on these.

knight-of-ni · 2014-10-15T18:49:33Z

Just confirmed that master (still) builds fine on Fedora & CentOS using ffmpeg.
Also built Fedora with the NO_FFMPEG flag. We are green.

Sune1337 · 2014-10-15T23:14:55Z

i wish i had champagne

Sune1337 added 3 commits April 25, 2014 22:12

A variable was referenced as a pointer when it was not.

4767326

- Removed an unreferenced variable

3155d62

- fixed compilation issue with sws_..isSupported when compiling with ffmpeg 0.5

Sune1337 added 2 commits April 25, 2014 23:22

- use _AVCODECID_NONE instead of CODEC_ID_NONE

448d287

- render video using double buffers

a4f5976

this is to deliver frames more accurately to the client

mastertheknife reviewed Apr 28, 2014
View reviewed changes

- Arrays should be deleted using "delete[]" and not "delete"

48b361b

knight-of-ni added this to the 1.28.0 milestone Oct 10, 2014

Sune1337 mentioned this pull request Oct 11, 2014

mpeg stream instead of jpeg is crashing (seg fault) on 1.26.4 [$10] #247

Closed

knight-of-ni merged commit 48b361b into ZoneMinder:master Oct 11, 2014

knight-of-ni mentioned this pull request Nov 8, 2015

Release v1.29.0 (take two) #1115

Closed

Zms/videostream improvements #404

Zms/videostream improvements #404

Conversation

Sune1337 commented Apr 25, 2014

Sune1337 commented Apr 25, 2014

Sune1337 commented Apr 25, 2014

Sune1337 commented Apr 25, 2014

mastertheknife Apr 28, 2014

Choose a reason for hiding this comment

Sune1337 commented Apr 28, 2014

knight-of-ni commented Apr 29, 2014

knight-of-ni commented Oct 10, 2014

Sune1337 commented Oct 10, 2014

Sune1337 commented Oct 11, 2014

Sune1337 commented Oct 11, 2014

Sune1337 commented Oct 11, 2014

knight-of-ni commented Oct 11, 2014

Sune1337 commented Oct 11, 2014

knight-of-ni commented Oct 11, 2014

Sune1337 commented Oct 11, 2014

knight-of-ni commented Oct 11, 2014

knight-of-ni commented Oct 11, 2014

knight-of-ni commented Oct 11, 2014

knight-of-ni commented Oct 11, 2014

Sune1337 commented Oct 11, 2014

Sune1337 commented Oct 15, 2014

Sune1337 commented Oct 15, 2014

Sune1337 commented Oct 15, 2014

knight-of-ni commented Oct 15, 2014

Sune1337 commented Oct 15, 2014

Sune1337 commented Oct 15, 2014

Sune1337 commented Oct 15, 2014

Sune1337 commented Oct 15, 2014

knight-of-ni commented Oct 15, 2014

Sune1337 commented Oct 15, 2014

Sune1337 commented Oct 15, 2014

knight-of-ni commented Oct 15, 2014

knight-of-ni commented Oct 15, 2014

Sune1337 commented Oct 15, 2014

knight-of-ni commented Oct 15, 2014

Sune1337 commented Oct 15, 2014

Sune1337 commented Oct 15, 2014

Sune1337 commented Oct 15, 2014

knight-of-ni commented Oct 15, 2014

Sune1337 commented Oct 15, 2014

Sune1337 commented Oct 15, 2014

knight-of-ni commented Oct 15, 2014

Sune1337 commented Oct 15, 2014

knight-of-ni commented Oct 15, 2014

Sune1337 commented Oct 15, 2014

Sune1337 commented Oct 15, 2014

connortechnology commented Oct 15, 2014

knight-of-ni commented Oct 15, 2014

Sune1337 commented Oct 15, 2014