| Message ID | 4eb04b91-3007-bab8-c377-9ee3ab70ae59@carpalis.nl |
|---|---|
| State | New |
| Headers | show |
2018-05-18 17:06 GMT+02:00, Jerome Borsboom <jerome.borsboom@carpalis.nl>: > This patch set solves various issues that affected the SA10180.vc1 > test file. With these patches applied, this file decodes bitequal to > the Intel VAAPI decoder on Haswell. I still see artefacts beginning after ~22 seconds that are not visible with the reference decoder, the first 547 frames are bit-exact. Carl Eugen
> 2018-05-18 17:06 GMT+02:00, Jerome Borsboom <jerome.borsboom at carpalis.nl>: > >> This patch set solves various issues that affected the SA10180.vc1 >> test file. With these patches applied, this file decodes bitequal to >> the Intel VAAPI decoder on Haswell. > > I still see artefacts beginning after ~22 seconds that are not visible > with the reference decoder, the first 547 frames are bit-exact. > > Carl Eugen I have found the issue that produces the artifacts. I need to update both ffmpeg and the Intel VAAPI driver as both make the same wrong assumption. Please expect at least one more patch in the coming days to resolve this issue. Regards, Jerome
On Fri, May 18, 2018 at 05:06:18PM +0200, Jerome Borsboom wrote: > FIELDTX bitplane is only present in interlace frame I pictures. > v->fieldtx_is_raw may spill over from a previous interlaced frame I picture > while decoding a non-interlace frame I picture. > > Signed-off-by: Jerome Borsboom <jerome.borsboom@carpalis.nl> > --- > This patch set solves various issues that affected the SA10180.vc1 test file. With > these patches applied, this file decodes bitequal to the Intel VAAPI decoder on Haswell. > > Please also review my patch set of May 9th that enables hwaccel decode of the SA10180.vc1 > file. > > libavcodec/vc1_block.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavcodec/vc1_block.c b/libavcodec/vc1_block.c > index f59c440943..daf30fdbfe 100644 > --- a/libavcodec/vc1_block.c > +++ b/libavcodec/vc1_block.c > @@ -2680,7 +2680,7 @@ static void vc1_decode_i_blocks_adv(VC1Context *v) > s->current_picture.motion_val[1][s->block_index[0] + v->blocks_off][1] = 0; > > // do actual MB decoding and displaying > - if (v->fieldtx_is_raw) > + if (v->fcm == ILACE_FRAME && v->fieldtx_is_raw) > v->fieldtx_plane[mb_pos] = get_bits1(&v->s.gb); fieldtx_is_raw is only set when fcm == ILACE_FRAME I suspect the intend was it is unset otherwise. This would avoid the extra check [...]
>> libavcodec/vc1_block.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavcodec/vc1_block.c b/libavcodec/vc1_block.c >> index f59c440943..daf30fdbfe 100644 >> --- a/libavcodec/vc1_block.c >> +++ b/libavcodec/vc1_block.c >> @@ -2680,7 +2680,7 @@ static void vc1_decode_i_blocks_adv(VC1Context *v) >> s->current_picture.motion_val[1][s->block_index[0] + v->blocks_off][1] = 0; >> >> // do actual MB decoding and displaying >> - if (v->fieldtx_is_raw) >> + if (v->fcm == ILACE_FRAME && v->fieldtx_is_raw) >> v->fieldtx_plane[mb_pos] = get_bits1(&v->s.gb); > > fieldtx_is_raw is only set when fcm == ILACE_FRAME > I suspect the intend was it is unset otherwise. This would avoid the extra > check I think this may be a design decision. You can either set the decoding context, 'v' in this case, to a sane default or only use syntax elements where appropriate. The current state of the bitstream decoder for VC-1 is not very clean in this regard. But it does certainly not reset the decoding context for each frame and even depends on this behaviour for at least one case. While I think it may be better to reset the decoding context to sane defaults for each frame for the applicable variables, for now, I would like to propose to leave the bitstream decoder as is and use this patch. Currently, I am focusing on compliance to the VC-1 spec and a cleanup of the bitstream decoder may be something down the road. A trac issue might be appropriate to remember this issue, although the decoder in general could use a good cleanup. Regards, Jerome
On Sun, May 20, 2018 at 10:56:19AM +0200, Jerome Borsboom wrote: > >> libavcodec/vc1_block.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/libavcodec/vc1_block.c b/libavcodec/vc1_block.c > >> index f59c440943..daf30fdbfe 100644 > >> --- a/libavcodec/vc1_block.c > >> +++ b/libavcodec/vc1_block.c > >> @@ -2680,7 +2680,7 @@ static void vc1_decode_i_blocks_adv(VC1Context *v) > >> s->current_picture.motion_val[1][s->block_index[0] + v->blocks_off][1] = 0; > >> > >> // do actual MB decoding and displaying > >> - if (v->fieldtx_is_raw) > >> + if (v->fcm == ILACE_FRAME && v->fieldtx_is_raw) > >> v->fieldtx_plane[mb_pos] = get_bits1(&v->s.gb); > > > > fieldtx_is_raw is only set when fcm == ILACE_FRAME > > I suspect the intend was it is unset otherwise. This would avoid the extra > > check > > I think this may be a design decision. You can either set the decoding > context, 'v' in this case, to a sane default or only use syntax elements > where appropriate. The current state of the bitstream decoder for VC-1 > is not very clean in this regard. But it does certainly not reset the > decoding context for each frame and even depends on this behaviour for > at least one case. > > While I think it may be better to reset the decoding context to sane > defaults for each frame for the applicable variables, for now, I would > like to propose to leave the bitstream decoder as is and use this patch. > Currently, I am focusing on compliance to the VC-1 spec and a cleanup of > the bitstream decoder may be something down the road. you are the one working on this so its clearly up to you to decide these things. Personally i would not leave fieldtx_plane at non zero value if that mode is not enabled > > A trac issue might be appropriate to remember this issue, yes > although the > decoder in general could use a good cleanup. yes thx [...]
diff --git a/libavcodec/vc1_block.c b/libavcodec/vc1_block.c index f59c440943..daf30fdbfe 100644 --- a/libavcodec/vc1_block.c +++ b/libavcodec/vc1_block.c @@ -2680,7 +2680,7 @@ static void vc1_decode_i_blocks_adv(VC1Context *v) s->current_picture.motion_val[1][s->block_index[0] + v->blocks_off][1] = 0; // do actual MB decoding and displaying - if (v->fieldtx_is_raw) + if (v->fcm == ILACE_FRAME && v->fieldtx_is_raw) v->fieldtx_plane[mb_pos] = get_bits1(&v->s.gb); cbp = get_vlc2(&v->s.gb, ff_msmp4_mb_i_vlc.table, MB_INTRA_VLC_BITS, 2); if (v->acpred_is_raw)
FIELDTX bitplane is only present in interlace frame I pictures. v->fieldtx_is_raw may spill over from a previous interlaced frame I picture while decoding a non-interlace frame I picture. Signed-off-by: Jerome Borsboom <jerome.borsboom@carpalis.nl> --- This patch set solves various issues that affected the SA10180.vc1 test file. With these patches applied, this file decodes bitequal to the Intel VAAPI decoder on Haswell. Please also review my patch set of May 9th that enables hwaccel decode of the SA10180.vc1 file. libavcodec/vc1_block.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)