Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#4842 closed defect (invalid)

wrong mask in dxv

Reported by: projectsymphony Owned by:
Priority: normal Component: avcodec
Version: git-master Keywords: dxv
Cc: Blocked By:
Blocking: Reproduced by developer: no
Analyzed by developer: no

Description

for some reason 9b2802f0d3296059a61b0f323ee0fc86eba48bf5 was applied without any explanation

besides the fact that the decoder has been tested across several "real-world" files, the test is clearly wrong and could lead to awful results.

if (old_type & 0x20 || old_type & 0x2)

should definitely be

if ((old_type & 0x20) || (old_type & 0x2))

would it be possible to have one of the samples that made this change necessary?

Attachments (1)

0001-dxv-Adjust-old-version-check.patch (971 bytes) - added by projectsymphony 4 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 4 years ago by cehoyos

  • Keywords dxv added
  • Resolution set to invalid
  • Status changed from new to closed

This ticket does not look valid (or correct) to me.

Concerning your question, sorry, my only answer is the same as when the new asf demuxer was presented: Where do you search for samples when (or after) implementing a new feature?

comment:2 follow-up: Changed 4 years ago by projectsymphony

Sorry if I wasn't clear with my ticket description.

The suggested change was only for clarity.

All samples I have (which come from "real-world") do not feature this header you found. It would be interesting if I could check it since it could contain further information in the headers that I haven't been able to reverse.

Would be able to provide one so that the decoder could improve further?

comment:3 Changed 4 years ago by cehoyos

I will point to the sample I used, please try to answer my question first, I believe it is really important to understand the difference between our projects / our attitude.

I am definitely thankful that you are looking at my patch, my original thought was that you just made a typo, it would be good if you could double-check.

Last edited 4 years ago by cehoyos (previous) (diff)

comment:4 in reply to: ↑ 2 Changed 4 years ago by cehoyos

Replying to projectsymphony:

Sorry if I wasn't clear with my ticket description.

The suggested change was only for clarity.

Allow me to add that "only for clarity" and "clearly wrong" are not synonyms.

comment:5 Changed 4 years ago by cehoyos

I believe the sample in question can be found in https://samples.libav.org/V-codecs/DXDI/
If the binary decoder just does } else { then imo this is what libavcodec should do.

comment:6 Changed 4 years ago by projectsymphony

Thanks for the sample. I was able to confirm that the sample is valid and it comes from the very first version of the codec (which supported only a single decoding mode, hence the absence of the else in the binary).

The check could be either removed entirely, or it could be set to test the encoder version exclusively. I think would prefer the latter so here is attached a patch that does that.

Version 0, edited 4 years ago by projectsymphony (next)

Changed 4 years ago by projectsymphony

comment:7 Changed 4 years ago by cehoyos

As said, I believe you should copy the binary decoders' behaviour, but you are the maintainer, so it is currently your decision.

Note: See TracTickets for help on using tickets.