Improvements for Rockchip ffmpeg #4
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This adds hardware decoding for h264 / HEVC / VP8 using MPP Rockchip API. Will return frames holding an AVDRMFrameDescriptor struct in buf[0] that allows drm / dmabuf usage. Was tested on RK3288 (TinkerBoard) and RK3328. Changes from Previous patch : - Frame colorspace info is now filled. - Frame interlacing is now filled (Note : currently had a bug in mpp which will be fixed soon by rockchip, will set the to progressive). - hw_frame_context returns none as format for the rockchip specific 10 bits. - Added support for VP9 codec. - removed MPG4 codec : it seems that MPP doesn't handle properly all the MPEG4 formats - removed MPEG2 : there is still an issue with MPEG2 decoder, this is being investigated by RockChip. - the ION format has been kept for MPP init (rather than DRM) as this is the only one working right, using DRM format will cause assertions upon close. - Other minor comments have been taken into account
|
Thnaks for that PR , but the patch was sent upstream :) So if you want to make imporvements about all this you probably want to send you patches to ffmpeg :) |
LongChair
reviewed
Oct 5, 2017
libavcodec/rkmppdec.c
Outdated
|
|
||
| // make sure we keep decoder full | ||
| if (freeslots > 1 && decoder->first_frame) | ||
| return AVERROR(EAGAIN); |
There was a problem hiding this comment.
Why would this be unnecessary ?
libavcodec/rkmppdec.c
Outdated
| ret = decoder->mpi->control(decoder->ctx, MPP_DEC_SET_PARSER_SPLIT_MODE, ¶mS32); | ||
| if (ret != MPP_OK) { | ||
| av_log(avctx, AV_LOG_ERROR, "Failed to set parser split mode on MPI (code = %d).\n", ret); | ||
| ret = AVERROR_UNKNOWN; | ||
| goto fail; | ||
| } |
There was a problem hiding this comment.
I didn't notice that this would affect pts order, where dud you see that ?
libavcodec/rkmppdec.c
Outdated
| } | ||
| if (ret != MPP_OK) { | ||
| av_log(avctx, AV_LOG_ERROR, "can't get a frame frome decoder (code = %d)\n", ret); | ||
| goto fail; | ||
| } |
There was a problem hiding this comment.
This would happen almost at each start and was confirmed by rk, we don't want error log in normal conditions at each start
| mpp_frame_deinit(&mppframe); | ||
| usleep(10000); | ||
| goto retry_get_frame; | ||
| } |
There was a problem hiding this comment.
Mpp frames are all released upon avframe relase. Realeasing some here dosn't make sene
| ret = AVERROR_EXIT; | ||
| goto fail; | ||
| } | ||
|
|
There was a problem hiding this comment.
That return code is not appropriate here. Once again i have already upstreamed those changes and if you feel that a few things should be changed, you need to send those patches to ffmpeg upstream, as the reference code is there now :)
|
Am Donnerstag, den 05.10.2017, 04:57 +0000 schrieb Lionel CHAZALLON:
@LongChair commented on this pull request.
I don't understand why most of these changes are done. They wouldn't
be upstreamed in that form to ffmpeg anyways. My repo is not the
right place to send them now, as my code was already merged by ffmpeg
In libavcodec/rkmppdec.c:
> @@ -546,10 +546,6 @@ static int rkmpp_receive_frame(AVCodecContext
*avctx, AVFrame *frame)
return ret;
}
}
-
- // make sure we keep decoder full
- if (freeslots > 1 && decoder->first_frame)
- return AVERROR(EAGAIN);
Why would this be unnecessary ?
I think look if there is a frame on decoder out is the better solution.
If there a frame get it, if not EAGAIN.
In libavcodec/rkmppdec.c:
> ret = decoder->mpi->control(decoder->ctx,
MPP_DEC_SET_PARSER_SPLIT_MODE, ¶mS32);
if (ret != MPP_OK) {
av_log(avctx, AV_LOG_ERROR, "Failed to set parser split mode
on MPI (code = %d).\n", ret);
ret = AVERROR_UNKNOWN;
goto fail;
- }
I didn't notice that this would affect pts order, where dud you see
that ?
I log the pts for AV sync debug:https://github.com/zillevdr/vdr-plugin-
softhddevice/blob/drm/video.c#L10623
In libavcodec/rkmppdec.c:
> @@ -347,16 +347,9 @@ static int rkmpp_retrieve_frame(AVCodecContext
*avctx, AVFrame *frame)
// in order to let it time to complete it's init, then we sleep
a bit between retries.
retry_get_frame:
ret = decoder->mpi->decode_get_frame(decoder->ctx, &mppframe);
- if (ret != MPP_OK && ret != MPP_ERR_TIMEOUT && !decoder-
>first_frame) {
- if (retrycount < 5) {
- av_log(avctx, AV_LOG_DEBUG, "Failed to get a frame,
retrying (code = %d, retrycount = %d)\n", ret, retrycount);
- usleep(10000);
- retrycount++;
- goto retry_get_frame;
- } else {
- av_log(avctx, AV_LOG_ERROR, "Failed to get a frame from
MPP (code = %d)\n", ret);
- goto fail;
- }
+ if (ret != MPP_OK) {
+ av_log(avctx, AV_LOG_ERROR, "can't get a frame frome decoder
(code = %d)\n", ret);
+ goto fail;
}
This would happen almost at each start and was confirmed by rk, we
don't want error log in normal conditions at each start
Yes, this should be DEBUG not Error.
In libavcodec/rkmppdec.c:
> @@ -352,6 +351,18 @@ static int rkmpp_retrieve_frame(AVCodecContext
*avctx, AVFrame *frame)
goto fail;
}
+ if (decoder->eos_reached && !mpp_frame_get_eos(mppframe)) {
+ if (mppframe)
+ mpp_frame_deinit(&mppframe);
+ usleep(10000);
+ goto retry_get_frame;
+ }
Mpp frames are all released upon avframe relase. Realeasing some here
dosn't make sene
mpp_packet_set_eos(packet) is set on decoder input. Then should wait
for mpp_frame_get_eos(mppframe) on decoder output.
In libavcodec/rkmppdec.c:
> @@ -352,6 +351,18 @@ static int rkmpp_retrieve_frame(AVCodecContext
*avctx, AVFrame *frame)
goto fail;
}
+ if (decoder->eos_reached && !mpp_frame_get_eos(mppframe)) {
+ if (mppframe)
+ mpp_frame_deinit(&mppframe);
+ usleep(10000);
+ goto retry_get_frame;
+ }
+ if (mppframe && mpp_frame_get_eos(mppframe)) {
+ av_log(avctx, AV_LOG_DEBUG, "EOS frame found\n");
+ ret = AVERROR_EXIT;
+ goto fail;
+ }
+
That return code is not appropriate here.
I'm not sure. Is AVERROR_EOS the right solution?
Why rkmpp is not in AVHWAccel?
Once again i have already upstreamed those changes and if you feel
that a few things should be changed, you need to send those patches
to ffmpeg upstream, as the reference code is there now :)
Thank you for your good work!
Best regards Jens
… —
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c5
5493e4bb","name":"GitHub"},"entity":{"external_key":"github/LongChair
/FFmpeg","title":"LongChair/FFmpeg","subtitle":"GitHub
repository","main_image_url":"https://cloud.githubusercontent.com/ass
ets/143418/17495839/a5054eac-5d88-11e6-95fc-
7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent
.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-
b52498112777.png","action":{"name":"Open in
GitHub","url":"https://github.com/LongChair/FFmpeg"}},"updates":{"sni
***@***.*** commented on
#4"}],"action":{"name":"View Pull
Request","url":"#4 (comment)
treview-67267931"}}}
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.