Closed rogerhu closed 10 months ago
Hi Roger! Thanks for this PR, this is pretty interesting to see FLV and RTMP still alive and evolving in 2023. I see that this is marked as WIP. What's missing before it can be merged?
On my side, since I wasn't aware of these latest evolutions, I need to take a look at the spec to understand it, then I'll try to review the proposed changes to FLVMeta.
Also, would you please be able to share sample video files demonstrating these changes?
Regards, Marc
Hi Marc,
I added a few unit tests to try to show the changes. Take a look!
I've also included an FLV file as part of the PR for review purposes. I can remove it before it's merged!
FYI, I just pushed a commit to fix the audio/video typo you also fixed here. Reason is I just wanted to make sure the "build" github action still worked as expected, as the run cache was empty. Sorry this causes you to have to resolve the conflict 😅.
Please note that on Windows, which is a supported platform for FLVMeta, the current build script action does not support the Check-based tests yet.
However, on my Windows dev machine, I usually check out Check separately so I don't rely on find_package.
The bottom line is that I'm afraid the newly added #include <unistd.h>
and function usage will cause the build for the tests to fail on Windows.
One solution could be to add a CMake check for the unistd.h header and conditionally compile the tests you added so the old ones can still be run.
One solution could be to add a CMake check for the unistd.h header and conditionally compile the tests you added so the old ones can still be run.
I just stopped using unistd.h and moved everything to use fwrite()!
Official ffmpeg changes are here: https://github.com/FFmpeg/FFmpeg/commit/aa83654d940e7599ee641fc5d0fcd10e7decf277
FFMPeg 6.1 includes the enhanced RTMP spec (https://9to5linux.com/ffmpeg-6-1-heaviside-released-with-vaapi-av1-encoder-hw-vulkan-decoding)
Running 6.1 version, we can also create an FLV with this new standard:
pushd /tmp
wget https://sample-videos.com/video123/mp4/720/big_buck_bunny_720p_1mb.mp4
ffmpeg -i /tmp/big_buck_bunny_720p_1mb.mp4 -c:a aac -vcodec libx265 test.flv
Thanks to this tool, I found a minor bug in the ffmpeg implementation - submitting a patch to address - should show up on https://ffmpeg.org/pipermail/ffmpeg-devel/2023-November/thread.html:
Code changes proposed:
--- a/libavformat/flvenc.c
+++ b/libavformat/flvenc.c
@@ -540,10 +540,10 @@ static void
flv_write_codec_header(AVFormatContext* s, AVCodecParameters* par, i
avio_write(pb, par->extradata, par->extradata_size);
} else {
if (par->codec_id == AV_CODEC_ID_HEVC) {
- avio_w8(pb, FLV_IS_EX_HEADER |
PacketTypeSequenceStart | FLV_FRAME_KEY); // ExVideoTagHeader mode
with PacketTypeSequenceStart
+ avio_w8(pb, FLV_IS_EX_HEADER |
(PacketTypeSequenceStart << FLV_VIDEO_FRAMETYPE_OFFSET)); //
ExVideoTagHeader mode with PacketTypeSequenceStart
avio_write(pb, "hvc1", 4);
Running build/src/flvmeta -r -F /tmp/test.flv
:
Before:
--- Tag #2 at 0x190 (400) ---
* Video codec: HEVC
* Video frame type: coded frames
After:
--- Tag #2 at 0x190 (400) ---
* Video codec: HEVC
* Video frame type: sequence start
Thanks to this tool, I found a minor bug in the ffmpeg implementation - submitting a patch to address - should show up on https://ffmpeg.org/pipermail/ffmpeg-devel/2023-November/thread.html:
Disregard. I found a bug after re-reading the spec and realized that the packet type is the lower 4 bits. Pushed the fix.
- it doesn't look like the metadata updating code takes into account the FOURCC codec yet. It is part of the updated spec to write the value in the
videocodecid
property.
Ah yes! Good catch.
- some checks probably need to be added to take the new headers into account.
Yeah and there some additional warnings I think in general.
- video size computation needs to be implemented for the newly added codecs
Good call-out!
Apart from these small issues, great job, thank you for the work put into this PR 👍 FLVMeta isn't the easiest codebase to get into, and it's been around for quite a while.
Thank you for the helpful feedback!
Sorry for the lack of replies this week, life got in the way. I'll review the latest changes this weekend to hopefully merge this PR 🤞
- it doesn't look like the metadata updating code takes into account the FOURCC codec yet. It is part of the updated spec to write the value in the
videocodecid
property.
It looks like I just needed to update the info.h header to 32-bits to make things work! Pushed the changes.
--- a/src/info.h
+++ b/src/info.h
@@ -30,7 +30,7 @@ typedef struct __flv_info {
uint8 have_audio;
uint32 video_width;
uint32 video_height;
- uint8 video_codec;
+ uint32_be video_codec;
uint32 video_frames_number;
uint8 audio_codec;
uint8 audio_size;
FYI - Google has a talk about the changes here: https://www.youtube.com/watch?v=5745HMvARRs&t=797s
The Extended RTMP standard (https://github.com/veovera/enhanced-rtmp ) relies on the unused highest order bit of the video tag frame byte. If this bit is set to 1, then another 4 bytes are read from the stream, which will denote the codec type (e.g. AV1, HVEC, VP9). In addition, the existing lower half byte is then used to provide signaling information (e.g. packet sequence start, coded frames, MPEG2 TS frames, etc.)
Inspired by https://github.com/ossrs/srs/pull/3495/files#diff-b381246f7b6c2af916111db040dc6a1d52a5288f4c0d2b783f5a90b13ac0d5b8
You can also create your own FLV with H.265 encoding with FFMPEG 6.1 or above:
I'm also able to test by creating an FLV file from using this Android library that incorporates these changes:
https://github.com/ThibaultBee/StreamPack/commit/3b4fe5b414f1b60f31cd2636ba45b1040445d1b4