Opened 10 years ago
Closed 10 years ago
#3895 closed defect (needs_more_info)
Multiple issues around the new function of -max_error_rate
| Reported by: | JayBlanc | Owned by: | |
|---|---|---|---|
| Priority: | important | Component: | ffmpeg |
| Version: | git-master | Keywords: | regression |
| Cc: | Blocked By: | ||
| Blocking: | Reproduced by developer: | no | |
| Analyzed by developer: | no |
Description
The new function -max_error_rate provides a way of issuing a failure return value if there are too many errors encountered during operation.
However the implementation has the following flaws,
- It is an undocumented change into the basic function of the CLI tool, and as such breaks many scripted and embedded uses of ffmpeg that do not expect positive return values from non-fatal errors.
- The 'percentage' does not seem to be very well quantified, as to if this means recoverable or non-recoverable errors. Media that decodes perfectly, but has packets that had errors, can trigger this. ie, poorly encoded but functionally decodable media, such as a flash svc2 file from a third party encoder that is decoded correctly but has repeated errors. Or streams that have redundancy or built in error correction, such as Over The Air TS streams.
My recommended change would be to make it default to a value of 1, and for errors to only be counted towards the percentage level if they are non-recoverable.
Attachments (1)
Change History (12)
comment:3 by , 10 years ago
Unfortunately, the sample media I have that shows this are all multi-gig sized and licensed, and for obvious reasons I can't just send slices of the parts that trigger it. Use a sample that has recoverable errors, and enough that it triggers -max_error_rate. Prior to the creation of -max_error_rate, the program would only return an error value on fatal errors.
However, this is not a regression. It was a deliberate design change, but one that seems to have unintentionally changed default behaviour in a bad way. Now default behaviour is to return an error value after a certain number of 'errors' has occurred, and it is difficult to identify what errors are being counted and to what threshold they are being measured. This breaks compatibility with pre-existing scripts and embedded use of the CLI tool.
Again, the requested work-around change is to make the default behaviour match the previous behaviour, and default -max_error_rate to 1. Which would restore compatibility for scripts and embedded uses of the CLI which do not expect -max_error_rate generated return values, and would only produce such return values when requested to do so.
comment:5 by , 10 years ago
Repeating myself: Unfortunately, the sample media I have that shows this are all multi-gig sized and licensed
by , 10 years ago
| Attachment: | ffmpeg-max_error_rate-default-1.patch added |
|---|
Patch to restore default behaviour to only having positive return values after a fatal error.
comment:6 by , 10 years ago
This patch should fix the issue, and prevent compatibility problems with scripts and embedded systems that do not expect error return values on non-fatal errors.
comment:7 by , 10 years ago
I don't think that any sample ever uploaded was not licensed (and due to its size it will be kept private anyway) and as said there is no file size limit. To make the ticket valid, please add the command line that allows to reproduce the regression together with the complete, uncut console output (current FFmpeg git head).
As an alternative, please send your patch (made with git format-patch and mentioning Fixes ticket #3895 in the commit message) to the ffmpeg-devel mailing list. Patches are discussed there but usually ignored here.
follow-up: 9 comment:8 by , 10 years ago
Repeating myself again: Please do not ask me to saturate my uplink for a considerable amount of time doing something that could get me into legal difficulty. The problem as described explains the issue, and it is trivial to confirm by reading the source how the new behaviour works and what it's impact is on maintaining compatibility with previous versions that did not have a positive return value unless a fatal error occurred.
I've pointed out the problem, I've fixed the problem for you, I've submitted a patch. But I do not want to become a developer on this project and I do not have time to spend on arguing to defend this patch on the mailing list, and I do not want to have to sign up to yet another open source project's mailing list in order to submit a trivial patch.
Nor do I have time and resources to put together a sample and demonstration of this, because doing so is actually more complex and less informative than just telling you what's wrong with the current default and giving you a patch to fix it. There is no additional information you would get from what you are asking for, in fact it wouldn't even tell you anything about the return value. And creating a suitable sample for you would be a complicated matter that would take a lot longer than it took to write the attached patch that fixes the problem.
If you want to close this as invalid, so be it. I'll either maintain my own patch, or switch to a different project's offering.
comment:9 by , 10 years ago
| Component: | undetermined → ffmpeg |
|---|---|
| Keywords: | regression added |
| Priority: | normal → important |
| Status: | new → open |
Replying to JayBlanc:
Repeating myself again: Please do not ask me to saturate my uplink for a considerable amount of time doing something that could get me into legal difficulty. The problem as described explains the issue, and it is trivial to confirm by reading the source how the new behaviour works and what it's impact is on maintaining compatibility with previous versions that did not have a positive return value unless a fatal error occurred.
More than 2/3 of error in a stream is kind of surprising and the problem might be somewhere else in FFmpeg, that is the reason a sample was requested. May I ask what kind of errors you have?
I've pointed out the problem, I've fixed the problem for you, I've submitted a patch. But I do not want to become a developer on this project and I do not have time to spend on arguing to defend this patch on the mailing list, and I do not want to have to sign up to yet another open source project's mailing list in order to submit a trivial patch.
You do not need to register to the mailing list to send the patch; the reason this was requested is because the review is not simple in the Trac, and that's just not how the project does it.
Anyhow, you current patch is breaking something else: in particular, it's breaking in case decoding a picture fails (the CLI will exit 0 instead of !0). See Ticket #2405.
I agree with you that this is a regression and something should be done about it, but your patch is unfortunately not the correct solution. Maybe the picture-case should be handled specially.
comment:10 by , 10 years ago
I dont think this can be fixed without a sample / testcase to reproduce the issue
comment:11 by , 10 years ago
| Resolution: | → needs_more_info |
|---|---|
| Status: | open → closed |
Please reopen this ticket if you can provide a test case.
It's also unclear from --help text if the percentage refers to percentage of time, percentage of frames, percentage of packets or percentage of unicorns.