You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by sreid8 <gi...@git.apache.org> on 2018/03/10 16:15:36 UTC
[GitHub] guacamole-server pull request #159: GUACAMOLE-465: beginning to support new ...
GitHub user sreid8 opened a pull request:
https://github.com/apache/guacamole-server/pull/159
GUACAMOLE-465: beginning to support new codecs and containers
This patch adds a dependency to "libavformat", the ffmpeg library that handles writing video and audio streams properly into video/audio containers. The patch makes NO changes to the way that guacenc functions from the user perspective.
This change is required to eventually support more codecs and containers. Previously, guacenc wrote encoded video frames directly into an output file. This works perfectly file for mpeg4 in an m4v container, but will not work for most other combinations (like h.264 in mp4, for example). However, this patch does not add any support for any new codecs or containers. After this patch, guacenc will still _only_ support mpeg4 in m4v.
This is patch 1 of 3 that will be provided for GUACAMOLE-465. The code for the other patches is written already on a branch that I've been maintaining, but in the interest in getting the functionality merged, I'm breaking it into multiple patches to make it easier to review.
**The current plans for the next patches are as follows:**
- Patch 2 will add the ability for the user to decide to specify input and output files rather that using the current batch interface, but will still provide the batch interface.
- Patch 3 will add h.264, VP8, and h.265 codecs in addition to the already existing mpeg4 and will also add mp4 and webm container support in addition to the already existing m4v support.
I welcome any suggestions for changes for this to be merged and will answer any questions about decisions I made while writing the patch. This _will_ require changes to the Guacamole Manual, but I didn't see those documents in the repo, so I haven't modified them myself. If those documents are open and accept pull requests, I will gladly update them myself as part of this patch.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sreid8/guacamole-server guacenc_libavformat
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/guacamole-server/pull/159.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #159
----
commit 35531c1e3d9bb8da15745834411292f0e2e3bf39
Author: Sean Reid <se...@...>
Date: 2018-03-10T15:57:32Z
GUACAMOLE-465: added dependency to libavformat as first step to supporting other types of codecs and containers in guacenc. migrated existing functionality to use the libavformat library for writing the files. there is not differnce to the user with this patch, but it provides a good base to finish this new feature from later
----
---
[GitHub] guacamole-server pull request #159: GUACAMOLE-465: beginning to support new ...
Posted by sreid8 <gi...@git.apache.org>.
Github user sreid8 commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/159#discussion_r173986084
--- Diff: src/guacenc/ffmpeg-compat.c ---
@@ -165,3 +197,62 @@ int guacenc_avcodec_encode_video(guacenc_video* video, AVFrame* frame) {
#endif
}
+AVCodecContext* guacenc_build_avcodeccontext(AVStream* stream,
+ AVCodec* codec,
+ int bitrate,
+ int width,
+ int height,
+ int gop_size,
+ int qmax,
+ int qmin,
+ int pix_fmt,
+ AVRational time_base) {
+#if LIBAVFORMAT_VERSION_INT < AV_VERSION_INT(57, 33, 100)
+ stream->codec->bit_rate = bitrate;
--- End diff --
In video.c, the stream is allocated already with the exact same codec that is passed here. It was only after libavformat 57.33.100 where the codec output parameters of the video were promoted from ```AVStream->codec->*``` to ```AVStream->codecpar```. The new function ```guacenc_open_avcodec``` handles moving the parameters from the ```context``` (which are set here) to ```codecpar``` in the case that libavformat >= 57.33.100. This ffmpeg API change is documented [here](https://github.com/FFmpeg/FFmpeg/blob/master/doc/APIchanges#L481).
However, before 57.33.100, the ```stream->codec``` is where the parameters are placed and the ```codec``` member of this struct has already been set to the proper codec when it was allocated.
---
[GitHub] guacamole-server pull request #159: GUACAMOLE-465: beginning to support new ...
Posted by sreid8 <gi...@git.apache.org>.
Github user sreid8 commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/159#discussion_r173988802
--- Diff: src/guacenc/ffmpeg-compat.c ---
@@ -165,3 +197,62 @@ int guacenc_avcodec_encode_video(guacenc_video* video, AVFrame* frame) {
#endif
}
+AVCodecContext* guacenc_build_avcodeccontext(AVStream* stream,
+ AVCodec* codec,
+ int bitrate,
+ int width,
+ int height,
+ int gop_size,
+ int qmax,
+ int qmin,
+ int pix_fmt,
+ AVRational time_base) {
+#if LIBAVFORMAT_VERSION_INT < AV_VERSION_INT(57, 33, 100)
+ stream->codec->bit_rate = bitrate;
+ stream->codec->width = width;
+ stream->codec->height = height;
+ stream->codec->gop_size = gop_size;
+ stream->codec->qmax = qmax;
+ stream->codec->qmin = qmin;
+ stream->codec->pix_fmt = pix_fmt;
+ stream->codec->time_base = time_base;
+#if LIBAVFORMAT_VERSION_INT < AV_VERSION_INT(55, 44, 100)
+ stream->codec->time_base = time_base;
+#else
+ stream->time_base = time_base;
+#endif
+ return stream->codec;
+#else
+ AVCodecContext* context = avcodec_alloc_context3(codec);
+ if (context) {
+ context->bit_rate = bitrate;
+ context->width = width;
+ context->height = height;
+ context->gop_size = gop_size;
+ context->qmax = qmax;
+ context->qmin = qmin;
+ context->pix_fmt = pix_fmt;
+ context->time_base = time_base;
+ stream->time_base = time_base;
--- End diff --
Similar to my other comment above about ```codec``` being ignored, it seems like ffmpeg is doing some soul searching about the roles of ```AVCodecContext``` and ```AVStream```. Just like the way that codec parameters are in ```AVStream->codecpar``` in >=57.33.100 instead of ```AVCodecContext->(parameter)``` or ```AVStream->codec->(parameter)```, it seems like they're having a hard time deciding where the ```time_base``` should be set. So since there are some versions where it needs to be on the ```AVCodecContext``` and some on the ```AVStream```, but no cases where it _can't_ be on both, I decided to put it on both to cover all our bases.
---
[GitHub] guacamole-server pull request #159: GUACAMOLE-465: beginning to support new ...
Posted by sreid8 <gi...@git.apache.org>.
Github user sreid8 commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/159#discussion_r173988127
--- Diff: src/guacenc/ffmpeg-compat.c ---
@@ -51,8 +51,41 @@
*/
static int guacenc_write_packet(guacenc_video* video, void* data, int size) {
- /* Write data, logging any errors */
- if (fwrite(data, 1, size, video->output) == 0) {
+ int ret;
+ AVPacket *pkt;
+
+
+#if LIBAVCODEC_VERSION_INT < AV_VERSION_INT(54,1,0)
+
+ pkt = malloc(sizeof(AVPacket));
+ /* have to create a packet around the encoded data we have */
+ av_init_packet(pkt);
+
+ if (video->context->coded_frame->pts != AV_NOPTS_VALUE) {
+ pkt->pts = av_rescale_q(video->context->coded_frame->pts,
+ video->context->time_base,
+ video->output_stream->time_base);
+ }
+ if (video->context->coded_frame->key_frame) {
+ pkt->flags |= AV_PKT_FLAG_KEY;
+ }
+
+ pkt->data = data;
+ pkt->size = size;
+ pkt->stream_index = video->output_stream->index;
+ ret = av_interleaved_write_frame(video->container_format_context, pkt);
+ free(pkt);
--- End diff --
Great point. Changed to a stack variable.
---
[GitHub] guacamole-server pull request #159: GUACAMOLE-465: beginning to support new ...
Posted by sreid8 <gi...@git.apache.org>.
Github user sreid8 commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/159#discussion_r173988068
--- Diff: src/guacenc/ffmpeg-compat.c ---
@@ -165,3 +197,62 @@ int guacenc_avcodec_encode_video(guacenc_video* video, AVFrame* frame) {
#endif
}
+AVCodecContext* guacenc_build_avcodeccontext(AVStream* stream,
+ AVCodec* codec,
--- End diff --
My mistake! I write Java all day at work and so the only excuse I can think of for why I didn't see this when I was eyeball-linting is that I must be too used to how long Java parameters tend to be pretty long. Consider it fixed. :disappointed:
---
[GitHub] guacamole-server pull request #159: GUACAMOLE-465: beginning to support new ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/159#discussion_r173687877
--- Diff: src/guacenc/ffmpeg-compat.c ---
@@ -165,3 +197,62 @@ int guacenc_avcodec_encode_video(guacenc_video* video, AVFrame* frame) {
#endif
}
+AVCodecContext* guacenc_build_avcodeccontext(AVStream* stream,
+ AVCodec* codec,
+ int bitrate,
+ int width,
+ int height,
+ int gop_size,
+ int qmax,
+ int qmin,
+ int pix_fmt,
+ AVRational time_base) {
+#if LIBAVFORMAT_VERSION_INT < AV_VERSION_INT(57, 33, 100)
+ stream->codec->bit_rate = bitrate;
+ stream->codec->width = width;
+ stream->codec->height = height;
+ stream->codec->gop_size = gop_size;
+ stream->codec->qmax = qmax;
+ stream->codec->qmin = qmin;
+ stream->codec->pix_fmt = pix_fmt;
+ stream->codec->time_base = time_base;
+#if LIBAVFORMAT_VERSION_INT < AV_VERSION_INT(55, 44, 100)
+ stream->codec->time_base = time_base;
+#else
+ stream->time_base = time_base;
+#endif
+ return stream->codec;
+#else
+ AVCodecContext* context = avcodec_alloc_context3(codec);
+ if (context) {
+ context->bit_rate = bitrate;
+ context->width = width;
+ context->height = height;
+ context->gop_size = gop_size;
+ context->qmax = qmax;
+ context->qmin = qmin;
+ context->pix_fmt = pix_fmt;
+ context->time_base = time_base;
+ stream->time_base = time_base;
--- End diff --
Can you describe why this function is touching `stream` as a side effect?
---
[GitHub] guacamole-server pull request #159: GUACAMOLE-465: beginning to support new ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/159#discussion_r173687760
--- Diff: src/guacenc/ffmpeg-compat.c ---
@@ -165,3 +197,62 @@ int guacenc_avcodec_encode_video(guacenc_video* video, AVFrame* frame) {
#endif
}
+AVCodecContext* guacenc_build_avcodeccontext(AVStream* stream,
+ AVCodec* codec,
+ int bitrate,
+ int width,
+ int height,
+ int gop_size,
+ int qmax,
+ int qmin,
+ int pix_fmt,
+ AVRational time_base) {
+#if LIBAVFORMAT_VERSION_INT < AV_VERSION_INT(57, 33, 100)
+ stream->codec->bit_rate = bitrate;
--- End diff --
Why is the `codec` parameter ignored in this case?
---
[GitHub] guacamole-server pull request #159: GUACAMOLE-465: beginning to support new ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/159#discussion_r173688157
--- Diff: src/guacenc/ffmpeg-compat.c ---
@@ -165,3 +197,62 @@ int guacenc_avcodec_encode_video(guacenc_video* video, AVFrame* frame) {
#endif
}
+AVCodecContext* guacenc_build_avcodeccontext(AVStream* stream,
+ AVCodec* codec,
--- End diff --
Please follow the general style of the surrounding code. This function stands out as the only one to dedicate a separate line to each parameter, regardless of line length.
---
[GitHub] guacamole-server pull request #159: GUACAMOLE-465: beginning to support new ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/159#discussion_r173687945
--- Diff: src/guacenc/ffmpeg-compat.c ---
@@ -51,8 +51,41 @@
*/
static int guacenc_write_packet(guacenc_video* video, void* data, int size) {
- /* Write data, logging any errors */
- if (fwrite(data, 1, size, video->output) == 0) {
+ int ret;
+ AVPacket *pkt;
+
+
+#if LIBAVCODEC_VERSION_INT < AV_VERSION_INT(54,1,0)
+
+ pkt = malloc(sizeof(AVPacket));
+ /* have to create a packet around the encoded data we have */
+ av_init_packet(pkt);
+
+ if (video->context->coded_frame->pts != AV_NOPTS_VALUE) {
+ pkt->pts = av_rescale_q(video->context->coded_frame->pts,
+ video->context->time_base,
+ video->output_stream->time_base);
+ }
+ if (video->context->coded_frame->key_frame) {
+ pkt->flags |= AV_PKT_FLAG_KEY;
+ }
+
+ pkt->data = data;
+ pkt->size = size;
+ pkt->stream_index = video->output_stream->index;
+ ret = av_interleaved_write_frame(video->container_format_context, pkt);
+ free(pkt);
--- End diff --
Why dynamically allocate an `AVPacket` only to immediately free it after use?
---