From patchwork Mon Dec 10 22:41:39 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mark Thompson X-Patchwork-Id: 11367 Return-Path: X-Original-To: patchwork@ffaux-bg.ffmpeg.org Delivered-To: patchwork@ffaux-bg.ffmpeg.org Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by ffaux.localdomain (Postfix) with ESMTP id ECECF44CAF3 for ; Tue, 11 Dec 2018 00:41:48 +0200 (EET) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 3C5E768A92A; Tue, 11 Dec 2018 00:41:39 +0200 (EET) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wm1-f66.google.com (mail-wm1-f66.google.com [209.85.128.66]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 9531E68A650 for ; Tue, 11 Dec 2018 00:41:33 +0200 (EET) Received: by mail-wm1-f66.google.com with SMTP id r24so7659903wmh.0 for ; Mon, 10 Dec 2018 14:41:44 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=jkqxz-net.20150623.gappssmtp.com; s=20150623; h=subject:from:to:references:message-id:date:user-agent:mime-version :in-reply-to:content-language:content-transfer-encoding; bh=V1RSEFu4dKo68oB+68vfljWeWE0Xdk1FItrpm8XMvxc=; b=ea6zLujI42nPPm6aEzPngKOsaPYnc4PNsMVWMDhGDCCqV8dAulC3GNEJHFmFyqBgj3 oK7pOQkTjucHxKCP9DtRC4F24HkG4HA+Kw/Asp6hM7PLXugOZmJ4ZGxQgUUszE8Z3d5L HM5nOOpKvNzFLqGesrPy1iOTXrTo4LQ9W8jnOJhvtdLzLUvoYlctGpQn8XaL/QTYf02L +H2602sp6wR3gUputsUxp3bLKF2ck59tyh5DAyIXeE43A1lEHHClG2W/PqtZvOubKyd+ /iOxtfhbAkdynCk/stbgm2cGk9ji9/URI2pvSk5qH4a78ViXA7n6mDf/wykLBOkUEr6w PSRg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:from:to:references:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=V1RSEFu4dKo68oB+68vfljWeWE0Xdk1FItrpm8XMvxc=; b=W1nWL2A1BK7inO9PnKbFEPtj4rN+ywxeafTDPittFg76StAg/oMekPECSDr868FLQ8 SCuxkputofFe/f0/6AgqZ4m3s7QlR4StsigJseA4XUVZ+eQx/z3IgzOugx/v9EWOdPG0 zQwpVgjGx10+r7TQSbePH5x4ZllnUvNNgjrM2jWB2CcL2ijIwH9KmPqEqzPRMbe3HkRc HNn3cKeYRNvEzxFw5B7XeSoIx1Fyeop3vprcbleRl8nQNZJlAp6g/0risDk6ILk6WPqg vEVQVxufcM42TCgF2tVPtbd4mf9Ff17oSp0qSUp1vZwuTKoxfTIFauRtOF3BT5lUHMXG QXRQ== X-Gm-Message-State: AA+aEWZeoEFGNxhORDiIaQREGfondEqvrv8C8ge6pmWg/fxKzb5nDyZ7 k8V7ChndaTIebIYi+1a2Bl+e4DV9X5c= X-Google-Smtp-Source: AFSGD/WCzSNyxWVCk0LT65k8cxqzAaVVXWGQV/YEF0np0nFU1bLsbLnemCH0U/fZ5WklDoVSwSTtVQ== X-Received: by 2002:a1c:4108:: with SMTP id o8mr174045wma.91.1544481703195; Mon, 10 Dec 2018 14:41:43 -0800 (PST) Received: from [192.168.0.4] (cpc91242-cmbg18-2-0-cust650.5-4.cable.virginm.net. [82.8.130.139]) by smtp.gmail.com with ESMTPSA id t4sm15314123wrm.6.2018.12.10.14.41.40 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 10 Dec 2018 14:41:41 -0800 (PST) From: Mark Thompson To: ffmpeg-devel@ffmpeg.org References: <20181206142641.3441-1-onemda@gmail.com> <20181207030431.GO3501@michaelspb> <20181207164043.GS3501@michaelspb> <2bfafa64-196c-e588-40dc-e13765978784@jkqxz.net> <937534b0-9532-f32c-bea9-a9deb57e7614@jkqxz.net> Message-ID: <925308a9-aaf5-a371-622b-1c73c7b508bb@jkqxz.net> Date: Mon, 10 Dec 2018 22:41:39 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.0 MIME-Version: 1.0 In-Reply-To: <937534b0-9532-f32c-bea9-a9deb57e7614@jkqxz.net> Content-Language: en-US Subject: Re: [FFmpeg-devel] [PATCH] avcodec/h264_refs: reset MMCO when invalid mmco code is found X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" On 09/12/2018 14:21, Mark Thompson wrote: > On 09/12/2018 13:54, Paul B Mahol wrote: >> On 12/9/18, Mark Thompson wrote: >>> On 09/12/2018 08:52, Paul B Mahol wrote: >>>> On 12/7/18, Paul B Mahol wrote: >>>>> On 12/7/18, Michael Niedermayer wrote: >>>>>> On Fri, Dec 07, 2018 at 10:36:23AM +0100, Paul B Mahol wrote: >>>>>>> On 12/7/18, Paul B Mahol wrote: >>>>>>>> On 12/7/18, Michael Niedermayer wrote: >>>>>>>>> On Thu, Dec 06, 2018 at 03:26:41PM +0100, Paul B Mahol wrote: >>>>>>>>>> This recovers state with #7374 linked sample. >>>>>>>>>> >>>>>>>>>> Work funded by Open Broadcast Systems. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Paul B Mahol >>>>>>>>>> --- >>>>>>>>>> libavcodec/h264_refs.c | 2 +- >>>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>>>>> >>>>>>>>>> diff --git a/libavcodec/h264_refs.c b/libavcodec/h264_refs.c >>>>>>>>>> index eaf965e43d..5645a203a7 100644 >>>>>>>>>> --- a/libavcodec/h264_refs.c >>>>>>>>>> +++ b/libavcodec/h264_refs.c >>>>>>>>>> @@ -718,6 +718,7 @@ int ff_h264_execute_ref_pic_marking(H264Context >>>>>>>>>> *h) >>>>>>>>>> } >>>>>>>>>> break; >>>>>>>>>> case MMCO_RESET: >>>>>>>>>> + default: >>>>>>>>>> while (h->short_ref_count) { >>>>>>>>>> remove_short(h, h->short_ref[0]->frame_num, 0); >>>>>>>>>> } >>>>>>>>>> @@ -730,7 +731,6 @@ int ff_h264_execute_ref_pic_marking(H264Context >>>>>>>>>> *h) >>>>>>>>>> for (j = 0; j < MAX_DELAYED_PIC_COUNT; j++) >>>>>>>>>> h->last_pocs[j] = INT_MIN; >>>>>>>>>> break; >>>>>>>>>> - default: assert(0); >>>>>>>>>> } >>>>>>>>>> } >>>>>>>>> >>>>>>>>> mmco[i].opcode should not be invalid, its checked around the point >>>>>>>>> where >>>>>>>>> this >>>>>>>>> array is filled. >>>>>>>>> unless there is something iam missing >>>>>>>> >>>>>>>> Yes, you are missing big time. >>>>>>>> If you think by "checked" about those nice asserts they are not >>>>>>>> enabled >>>>>>>> at >>>>>>>> all. >>>>>>>> >>>>>>> >>>>>>> There is check for invalid opcode, but stored invalid opcode is not >>>>>>> changed. >>>>>> >>>>>> Theres no question that we end with a invalid value in the struct, but >>>>>> that >>>>>> is not intended to be in there. You can see that this is not intended by >>>>>> simply looking at the variable that holds the number of entries, it is >>>>>> not written at all in this case. >>>>>> >>>>>> So for example if this code stores 5 correct looking mmcos and the 6th >>>>>> is >>>>>> invalid, 6 are in the array but the number of entries is just left where >>>>>> it >>>>>> was, that could be maybe 3 or 8 or 1. Just "defaulting out" the invalid >>>>>> value >>>>>> later doesnt feel ideal. >>>>> >>>>> Nope, mmco state is left in inconsistent state, and my patch fix it. As >>>>> you >>>>> provided nothing valuable to consider as alternative I will apply it. >>>>> >>>> >>>> What about converting any invalid mmco opcode to mmco reset and >>>> updating mmco size too? >>>> Currently mmco size is left in previous state. >>> >>> Don't invent a new RESET (= 5) operation - that type is special and its >>> presence implies other constraints which we probably don't want to think >>> about here (around frame_num in particular). >>> >>> I think END / truncating the list would be best option? >>> >> >> Nope, that would still put it into bad state. With your approach decoder does >> not recover from artifacts. Try sample from bug report #7374. > > Adding a spurious reset here throws away all previous reference frames, which will break the stream where it wouldn't otherwise be if the corrupted frame would have been bypassed for referencing. For example, in real-time cases with feedback a stream which has encountered errors can be recovered without an intra frame by referring to an older frame which still exists in the DPB. Sample: . The bad frame here has frame_num 24, but we already hit an error before that point and the feedback about that makes frame_num 25 refer to LTRF 1 such that 24 is never used. (From a simulator rather than a real capture, because random bit errors are never where you want them.) It doesn't exactly hit the original issue because the leftover MMCO count from the previous slice is not large enough. With: to make sure the MMCO count is written with a high enough value it does. The decoder then loses sync after the inserted reset when that is present and so all frames are wrong, while without the reset sync is maintained and all errors are fixed within a few round trips. - Mark diff --git a/libavcodec/h264_refs.c b/libavcodec/h264_refs.c index 5645a203a7..977b4ed12f 100644 --- a/libavcodec/h264_refs.c +++ b/libavcodec/h264_refs.c @@ -875,6 +875,7 @@ int ff_h264_decode_ref_pic_marking(H264SliceContext *sl, GetBitContext *gb, av_log(logctx, AV_LOG_ERROR, "illegal memory management control operation %d\n", opcode); + sl->nb_mmco = i + 1; return -1; } if (opcode == MMCO_END)