Re: [FFmpeg-devel] [ANN] Poll Results: GA voters list updates
Hi all, hi J-B! On 2023-11-06 07:10 +0100, Jean-Baptiste Kempf wrote: > Yo, > > Time is up, results are here: > https://vote.ffmpeg.org/cgi-bin/civs/results.pl?id=E_029f7195fed7aadf Should I have been mailed about this vote? I'm pretty sure I could vote in 2020. Or am I just missing something? Best regards, Alexander > The 1. place goes to > A: twice a year (1st Jan & 1st July, 0:00 UTC); as an exception, the list > will also be updated immediately after this vote > > The 2. place goes to: > B: before each vote > > The 3. place goes to: > D: keep everyone who had vote rights but add active developers each 1st Jan > and 1st July, 0:00 UTC; as an exception, the > list will also be updated immediately after this vote > > The 4. place goes to: > C: never (keep the 2020 version) > > The GA has spoken, so, option A is taken, with a bi-annual update. > > Patches for the documentation are welcome. > > Best, > > -- > Jean-Baptiste Kempf - Vice > +33 672 704 734 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] doc: add spi.txt
On 2023-10-16 00:49 +0200, Michael Niedermayer wrote: > This explains how to request refunds and what can be funded by SPI > > Co-Author: Stefano Sabatini > --- > doc/spi.txt | 73 + > 1 file changed, 73 insertions(+) > create mode 100644 doc/spi.txt > > diff --git a/doc/spi.txt b/doc/spi.txt > new file mode 100644 > index 00..ff4af8f42b > --- /dev/null > +++ b/doc/spi.txt > @@ -0,0 +1,73 @@ > +SPI (Software in the Public Interest) is a non-profit corporation > +registered in the state of New York founded to act as a fiscal sponsor > +for organizations that develop open source software and hardware. For > +details check here: > +https://www.spi-inc.org/ > + [...] > + > +Is it possible to fund active development by SPI: > +(the texts below have been taken from multiple > + replies FFmpeg has received from SPI, they have been edited > + so that "I" was replaced by "SPI" in some cases.) > +- > +Paying for development *does* require substantial > +additional paperwork, but it is not prohibited. > + > +Several SPI projects pay contractors for development > +efforts. SPI needs a contract in place which describes the work to be > +done. There are also various things SPI needs to check (e.g. are they a > +US person or not, as with GSoC mentor payments; are they really a > +contractor and not a employee). > + > +SPI can't deal with employment at the moment because that involves a > +lot of work, like health insurance, tax withholding, etc. Contractors > +are easier because they have to take care of that themselves; Whether > +someone is a contractor vs employee depends on various factors (that > +of course are different in every country) and can be disputed (see > +e.g. the debate about whether Uber drivers are employees); SPI has a > +questionnaire about their circumstances.) ^ Minor nit: Unbalanced parenthesis Can just be removed I read it correctly. Best regards, Alexander > +Unfortunately, there's no one-size-fits all when dealing with contractors. > +As already mentioned, without knowing the contributor's country > + > +SPI does have templates, but they depend on the contractors country. If it's > +US, Australia, France and a couple others SPI could provide them next day, > +otherwise SPI would need to ask their attorney to draft one, which would > +take some time > + > +Also, SPI has two models, MSA (which transfers ownership) and CSA (which > +grants a license instead). SPI usually sends the MSA (it's better for most > +purposes), but for development purposes, some projects prefer that the > +contractor retains ownership rights. > -- ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] avcodec/mpegvideo: Remove spec-incompliant inverse quantisation
On 2023-11-08 12:40 +0100, Anton Khirnov wrote: > Quoting Michael Niedermayer (2023-10-31 09:40:44) > > On Mon, Oct 30, 2023 at 02:11:27PM +0100, Andreas Rheinhardt wrote: > > > Section 7.4.4 of the MPEG-2 specifications requires that the > > > last bit of the last coefficient be toggled so that the sum > > > of all coefficients is odd; both our decoder and encoder > > > did this only if the bitexact flag has been set (although > > > stuff like this should be behind AV_CODEC_FLAG2_FAST). > > > This patch changes this by removing the spec-incompliant > > > functions. > > > > This commit message should include benchamarks documenting the speed loss > > (of the unquantize, the IDCT and overall) > > It is expected that the speed of some IDCTs will be impacted negativly > > as the non zero terms will prevent the skiping of some significant code > > > > as well as information about how much PSNR improves (to the encoder input) > > > > Also the change is a +-1 in one spot before the IDCT, the IDCT is not > > bitexactly > > specified in MPEG-2 so one could think of this as a > > correct implementation followed by a IDCT that was sometimes +-1 off > > instead of spec non compliance > > > > Only after the benchmarks and PSNR is presented should we decide if this > > is a change we want > > I disagree that the burden of proof should be on Andreas here. It should > be up to whoever wants to keep this code to show that it is useful. There was an argument presented. That argument could be challenged or otherwise explained why it more important to have this always behave like with bitexact. This could lead to "OK, I think removal is better" or if not benchmarks could lead to one or the other decision. Saying the burden is on whoever wants to keep the code sounds like a way for arbitrary code removal. While I agree getting rid of code can be a good thing, this would definitely take it too far. Best regards, Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] avcodec/mpegvideo: Remove spec-incompliant inverse quantisation
On 2023-11-09 11:13 +0100, Anton Khirnov wrote: > Quoting Alexander Strasser (2023-11-08 21:55:10) > > On 2023-11-08 12:40 +0100, Anton Khirnov wrote: > > > Quoting Michael Niedermayer (2023-10-31 09:40:44) > > > > On Mon, Oct 30, 2023 at 02:11:27PM +0100, Andreas Rheinhardt wrote: > > > > > Section 7.4.4 of the MPEG-2 specifications requires that the > > > > > last bit of the last coefficient be toggled so that the sum > > > > > of all coefficients is odd; both our decoder and encoder > > > > > did this only if the bitexact flag has been set (although > > > > > stuff like this should be behind AV_CODEC_FLAG2_FAST). > > > > > This patch changes this by removing the spec-incompliant > > > > > functions. > > > > > > > > This commit message should include benchamarks documenting the speed > > > > loss > > > > (of the unquantize, the IDCT and overall) > > > > It is expected that the speed of some IDCTs will be impacted negativly > > > > as the non zero terms will prevent the skiping of some significant code > > > > > > > > as well as information about how much PSNR improves (to the encoder > > > > input) > > > > > > > > Also the change is a +-1 in one spot before the IDCT, the IDCT is not > > > > bitexactly > > > > specified in MPEG-2 so one could think of this as a > > > > correct implementation followed by a IDCT that was sometimes +-1 off > > > > instead of spec non compliance > > > > > > > > Only after the benchmarks and PSNR is presented should we decide if this > > > > is a change we want > > > > > > I disagree that the burden of proof should be on Andreas here. It should > > > be up to whoever wants to keep this code to show that it is useful. > > > > There was an argument presented. > > I see no argument for why the code in question is useful, can you point > to the exact text? First this: > > > > It is expected that the speed of some IDCTs will be impacted negativly > > > > as the non zero terms will prevent the skiping of some significant code Second there was an argument for compliance: > > > > Also the change is a +-1 in one spot before the IDCT, the IDCT is not > > > > bitexactly > > > > specified in MPEG-2 so one could think of this as a > > > > correct implementation followed by a IDCT that was sometimes +-1 off > > > > instead of spec non compliance Third there was no rejection of the change, but a request for measurement of the effect. I would expect an approval of the patch if the measurement leads to insignificant enough results. > > That argument could be challenged or otherwise explained why it more > > important to have this always behave like with bitexact. > > > > This could lead to "OK, I think removal is better" or if not benchmarks > > could lead to one or the other decision. > > > > Saying the burden is on whoever wants to keep the code sounds like a way > > for arbitrary code removal. While I agree getting rid of code can be a good > > thing, this would definitely take it too far. > > All code is a maintenance burden, therefore all code should have a > reason for its presence in the codebase, otherwise it should be removed. I can't see how the reason for the presence of code can be ultimately defined objectively and non-arbitrary. Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] avcodec/mpegvideo: Remove spec-incompliant inverse quantisation
On 2023-11-08 17:58 -0500, Vittorio Giovara wrote: > On Wed, Nov 8, 2023 at 3:46 PM Alexander Strasser wrote: > > > On 2023-11-08 12:40 +0100, Anton Khirnov wrote: > > > Quoting Michael Niedermayer (2023-10-31 09:40:44) > > > > On Mon, Oct 30, 2023 at 02:11:27PM +0100, Andreas Rheinhardt wrote: > > > > > Section 7.4.4 of the MPEG-2 specifications requires that the > > > > > last bit of the last coefficient be toggled so that the sum > > > > > of all coefficients is odd; both our decoder and encoder > > > > > did this only if the bitexact flag has been set (although > > > > > stuff like this should be behind AV_CODEC_FLAG2_FAST). > > > > > This patch changes this by removing the spec-incompliant > > > > > functions. > > > > > > > > This commit message should include benchamarks documenting the speed > > loss > > > > (of the unquantize, the IDCT and overall) > > > > It is expected that the speed of some IDCTs will be impacted negativly > > > > as the non zero terms will prevent the skiping of some significant code > > > > > > > > as well as information about how much PSNR improves (to the encoder > > input) > > > > > > > > Also the change is a +-1 in one spot before the IDCT, the IDCT is not > > bitexactly > > > > specified in MPEG-2 so one could think of this as a > > > > correct implementation followed by a IDCT that was sometimes +-1 off > > > > instead of spec non compliance > > > > > > > > Only after the benchmarks and PSNR is presented should we decide if > > this > > > > is a change we want > > > > > > I disagree that the burden of proof should be on Andreas here. It should > > > be up to whoever wants to keep this code to show that it is useful. > > > > There was an argument presented. > > > > That argument could be challenged or otherwise explained why it more > > important to have this always behave like with bitexact. > > > > This could lead to "OK, I think removal is better" or if not benchmarks > > could lead to one or the other decision. > > > > Saying the burden is on whoever wants to keep the code sounds like a way > > for arbitrary code removal. While I agree getting rid of code can be a good > > thing, this would definitely take it too far. > > > > To be fair, this is noncompliant spec code, it shouldn't be present at all > since it produces inconsistent results (with the spec) and there is no > device particularly needing this functionality.It's not arbitrary code > removal, it's removing something that is not needed any more since the > speed impact (pro or against) is negligible on modern computers. Please see the reply to Anton. I hope it's clearer now what I meant. > I'm of the opinion that presenting an argument against such a targeted and > specific code removal with no supportive use case should be noted but not > acted upon, until relevant proof is brought over. Yes sadly that burden > should fall on whoever is presenting the argument. I wouid disagree in general. Maybe this case is more special (targeted and specific?) then my current understanding of the matter. IIUC the leading reason for this patch is removing the code because of spec incompliance which I think is not so clear and could be argued both ways. Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [ANN] Poll Results: GA voters list updates
Hi Anton! On 2023-11-09 13:15 +0100, Anton Khirnov wrote: > Quoting Alexander Strasser (2023-11-06 16:56:55) > > On 2023-11-06 07:10 +0100, Jean-Baptiste Kempf wrote: > > > Yo, > > > > > > Time is up, results are here: > > > https://vote.ffmpeg.org/cgi-bin/civs/results.pl?id=E_029f7195fed7aadf > > > > Should I have been mailed about this vote? > > > > I'm pretty sure I could vote in 2020. Or am I just missing something? > > As per the rules: > > Additional members are added to the General Assembly through a vote > > after proposal by a member of the General Assembly. They are part of > > the GA for two years, after which they need a confirmation by the GA. > > your voting rights expired after 2 years and were not renewed, since > nobody organized a new vote. Interesting. That could be the reason why I was left out. Not sure it completely fits to the information so far. In any way it's worth pointing out. Thanks. What I don't understand: > after which they need a confirmation by the GA 1. Does that mean all additional members drop out and need a confirmation by the remaining GA? 2. What does it exactly mean to need an confirmation? Does each additional member need to ask for a confirmation after 2 years? Or should there be a GA vote, every 2 years, for all additional members that would be dropped (after asking them if they want to stay in the GA)? 3. In which way is point 2 to be co-ordinated with new votes after 2 years? The text you quoted (according to my Git archaeology it wasn't changed; only reformatted) is unfortunately a bit ambiguous and unclear :( Best regards, Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [ANNOUNCE] upcoming vote: extra members for GA
On 2023-11-09 12:55 +0100, Michael Niedermayer wrote: > On Thu, Nov 09, 2023 at 10:44:12AM +0100, Anton Khirnov wrote: > > As nobody expressed a preference, the vote will start next Monday > > (2023-11-13). It should run for a week, and will be followed by TC/CC > > elections. > > > > The only extra GA candidate I see proposed so far is Ronald. If anyone > > wants to suggest further people, please do so in this thread ASAP. > > IMHO the question of the relation of the list of people who could vote > in the last GA vote and the people who where in the general assembly > in 2020 should be awnsered before further votes. > https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2023-November/316609.html > > If the current GA stays as it is, then i propose the following people > (this list was quickly made and certainly has omisions and possibly errors, > check anything thats important to you!) > > The presumed 2020 General assembly as well as people who have subtantial > contributions over the lifetime of the project where the source of this list > > Fabrice Bellard(Founder of the project over 600 commits in FFmpeg) > Aman Karmani (17 authored commits in 2020-2023, recently active in > 2023-June and work all over the codebase) > Baptiste Coudurier (Pays for our fate server since forever, maintains 15 > things in MAINTAINERs, is the the copyright of 56 files, over 1900 commits in > FFmpeg) > Moritz Barsnick(Member of the 2020 GA but was not on jbs list) > Lauri Kasanen (Linux / PowerPC maintainer) > Dale Curtis(14 commits in 2020 was in the 2020-GA) > Alexander Strasser (Root admin, just recently reported that he could not vote > even though he was in teh 2020-GA, 3 things in MAINTAINERs) I want to be an additional (extra) member in the GA. I have * commits in FFmpeg and reviewed patches (authored myself and applied for others) * spent time and effort in discussions on the mailing lists, issue trackers and IRC channels * admin rights to some infra structure of the project I also took part and helped organize FFmpeg representation in quite some FOSS conferences in Germany. Also was a few times at VDD. My first contact to the general multimedia community was around 2000. The first interaction with open source multimedia projects was around 2004. In the recent times I have not participated on the MLs (and IRC) much as I don't have much spare time after work. I'm hoping to continue supporting the project and help it stay alive and as awesome as it has been already over the many years since it took off. Thanks for your consideration, Alexander Strasser > foo86 (maintainer and author of dca*, dolby_e*, dtsdec, s337m) > Huiwen Ren (8 matches in the codebase of his name, avs2* and related > things maintainer) > Martin Vignali (over 200 commits in FFmpeg, left in 2019 because of > hostility) > Lou Logan (over 100 commits in FFmpeg, left in 2020, also in the > 2020-GA but not on jbs list) > Matthieu Bouron(over 200 commits in FFmpeg, recently active in 2022, 5 > entries in MAINTAINERs, 25 in copyrights) > Ruiling Song (vf_tonemap_opencl maintainer, added SIMD code to various > bits of code, last active in git in 2020) > John Stebbins (over 100 commits in FFmpeg, last active in git in 2020, 1 > copyright match) > Ting Fu(last active in April 2023, 24 commits in 2021-2023, ) > Tobias Rapp(vf_readvitc mainatiner, last commit in Jul 2023, 88 > commits in FFmpeg) > Shiyou Yin (MIPS & LoongArch maintainer, 24 matches for his name in > the source, last commit May 2023) > Reimar Döffinger (over 1400 commits in FFmpeg, root admin and last commit > in Oct 2023, 49 matches for his name in the codebase) > Aurelien Jacobs(over 1000 commits in FFmpeg, 13 entries in MAINTAINERs, > 70 copyright line matches) > Hendrik Leppkes(over 900 commits in FFmpeg, 2 entries in MAINTAINERs, 4 > copyrights, last commit in Jul 2023) > Vitor Sessak (over 900 commits in FFmpeg, 28 copyright matches) > Kostya Shishkov(over 900 commits in FFmpeg, 162 copyright matches) > Ramiro Polla (over 600 commits in FFmpeg, 5 matches in MAINTAINERs, 22 > copyright matches) > Alex Converse (over 500 commits in FFmpeg, 32 copyright matches) > Janne Grunau (over 500 commits in FFmpeg, 24 copyright matches) > Andreas Cadhalpun (over 400 commits in FFmpeg, 2 copyright matches) ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] tools/general_assembly: implement extra GA members
On 2023-11-26 10:18 +0100, Anton Khirnov wrote:
> Set pushed.
>
> The general_assembly.pl script should now be usable as the authoritative
> source for GA members.
The patches mostly LGTM.
My Perl knowledge in general is really mostly from 20 years ago.
So if there is any Perl-ish devil in the details I surely have
overlooked it.
Please pardon me if I missed any details on how the program works.
The edge cases are always the tricky stuff...
One thing about this patch and that program in general is a bit
unfortunate: The use of PerlDate is_between.
Here is the doc I found for it:
$dt->is_between( $lower, $upper )
Checks whether $dt is strictly between two other DateTime objects.
"Strictly" means that $dt must be greater than $lower and less than $upper.
If it is equal to either object then this method returns false.
AFAIU it affects the script in 2 places:
1. In subroutine get_date_range:
Here the exact day matching date_ga_rule is treated like
anything >= date_first_regular
2. In the loop adding the extra member. The member would not be added
on both, the day they were elected nor the day 2 years after.
Case 1 should be "strictly academical" and thus not really important
because to my knowledge no vote was started on that day.
For case 2 it will be not important on most days, but it would seem
more common and intuitive to use either the closed interval or a
half open interval. Where including the first and the last day or
including the first and excluding the day seem most natural to me.
Best regards,
Alexander
P.S.
As date calculations always turn out nightmares if you look at them
long enough, it would possible be a good idea to always use UTC and
review how time zones are handled in git CLI.
P.P.S.
For quick reference follow copies for both places referenced above:
Case 1:
sub get_date_range {
my ($now) = @_;
# date on which the GA update rule was established, and the voter list
# was extraordinarily updated; cf.:
# *
http://lists.ffmpeg.org/pipermail/ffmpeg-devel/2023-October/316054.html
# Message-Id
<169818211998.11195.16532637803201641...@lain.khirnov.net>
# *
http://lists.ffmpeg.org/pipermail/ffmpeg-devel/2023-November/316618.html
# Message-Id
<5efcab06-8510-4226-bf18-68820c7c6...@betaapp.fastmail.com>
my $date_ga_rule = DateTime->new(year => 2023, month => 11, day
=> 06);
# date when the regular update rule is first applied
my $date_first_regular = DateTime->new(year => 2024);
if ($now->is_between($date_ga_rule, $date_first_regular)) {
return ($date_ga_rule->clone()->set_year($date_ga_rule->year - 3),
$date_ga_rule);
}
if ($now < $date_ga_rule) {
print STDERR "GA before $date_ga_rule is not well-defined, be very
careful with the output\n";
}
my $cur_year_jan = $now->clone()->truncate(to => "year");
my $cur_year_jul = $cur_year_jan->clone()->set_month(7);
my $date_until= $now > $cur_year_jul ? $cur_year_jul :
$cur_year_jan;
my $date_since= $date_until->clone()->set_year($date_until->year -
3);
return ($date_since, $date_until);
}
Case 2:
foreach my $entry (@extra_members) {
my $elected = $entry->[2];
if ($date->is_between($elected,
$elected->clone()->set_year($elected->year + 2))) {
$assembly{$entry->[0]} = $entry->[1];
}
}
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] fate.sh: Allow overriding what targets to make for running the tests
Hi Martin, patch looks technically good to me. On 2023-11-27 17:46 +0200, Rémi Denis-Courmont wrote: > Le maanantaina 27. marraskuuta 2023, 14.31.18 EET Martin Storsjö a écrit : > > This can be useful if doing testing of uncommon CPU extensions by > > running tests with QEMU (by configuring with e.g. > > "target_exec=qemu-aarch64"), by only running the checkasm tests, > > to get a reasonable test coverage without excessive test runtime. > > For the purpose of testing future or bleeding-edge CPU extensions on emulator, > you would normally want to be able to actually filter those in. That is more > of > a matter of patching checkasm than FATE. > > Considering the poor coverage of checkasm, I fear that this just gives the > wrong impression, not to say a false sense of security. It feels misleading to > encourage or support that paradigm into FATE, in light of that poor coverage. > Afterall, if it's just about running checkasm, anybody can just run > `make tests/checkasm/checkasm && tests/checkasm/checkasm`. Agree, the practice should not be encouraged over cases where it is necessary to be practical, but having fate clients run the tests and submit the results seems still worthwhile to me. We shouldn't get a false sense of security and it can be a problem. Improving checkasm and qemu and maybe getting actual bleeding edge hardware into fate could help with that. Not saying it is easy or effortless... Still having tests run and some failures detected seems better than nothing to me. It would be only problematic if the false positives or negatives weigh out the cases where it is helpful. @Remi: Please take no offense. This does not look like a black and white issue to me , thus I wanted to voice what I thought when initially saw the patch and your response. Best regards, Alexander > Either way, this feels like a case of cart before horse. > > Also FWIW, RV broke due to misaligned accesses and illegal vector types that > QEMU tolerated. That is rather an argument against QEMU than against this MR > but still. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [ANNOUNCE] upcoming vote: TC/CC elections
On 2023-11-30 00:14 +0100, Michael Niedermayer wrote: > On Tue, Nov 28, 2023 at 02:23:13PM +0100, Anton Khirnov wrote: > > For the record, I've edited the vote description to make it more clear. > > It now looks like this: > > > > Five people from the list below will become the members of the Technical > > Committee (TC). Assign weights to each person according to how much you > > want them to be in the committee (higher weight = higher preference). > > > > > The system will assume you want to maximise the sum of weights of > > selected candidates. E.g. if X is given a weight of 10 and Y and Z have > > weights 8 and 6 respectively, then the voting algorithm will assume you > > prefer a committee with both Y and Z over one with X, because 14 > 10. > > However, giving Y and Z weight of 4 and 2 instead would have expressed > > that X is preferred to a combination of Y and Z, because 6 < 10. > > My try in cooking word soup: > > The system will assume you want to maximise the sum of weights of the > selected candidates. > > Some examples: > If you give Jerry a weight of 10 and give Tom a weight of 9, that means > you prefer Jerry over Tom because 10 > 9 > If you give Spike a weight of 20 that would mean you not only prefer Spike > over Tom OR Jerry but also over Tom AND Jerry. Because 20 > 10 + 9 > > OTOH if you give Spike a weight of 18 that would mean you prefer Spike over > Tom OR Jerry but you prefer Tom AND Jerry over Spike. > Because: 9 < 10< 18 < 9 + 10 > Tom < Jerry < Spike < Tom and Jerry This is similar to the improved notice from Anton, but IMHO makes it a bit more tangible by using person names and by also explaining it in a longer way with AND, OR, and more spacing. Not sure how easy it is to add to the voting and get a proper layout/formatting like in this mail with fixed width though. Also not sure if combined-weights mode and the way it makes voting itself harder (at least the first time one is confronted with it) is worth to keep for the next vote. I personally have no strong opinion. This is not complaint. Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavd/v4l2: Return FFERROR_REDO when receiving a frame of unexpected size
Hi Stephan!
On 2019-03-25 20:32 +0100, Stephan Hilb wrote:
> Alexander Strasser wrote on 21.03.2019 at 23:34:
>
> > 3. Return zero-sized packets => This works and is consistent with how
> >we handle frames flagged to be corrupted (V4L2_BUF_FLAG_ERROR).
> >See commit 28f20d2ff487aa589643d8f70eaf614b78839685 .
>
> I posted a patch for this on Sat, 25 Aug 2018. It seems to have been
> forgotten, attached again.
I didn't know you sent that patch.
> From 0af8515acca4d598570d03450656adc0ed7ac2d7 Mon Sep 17 00:00:00 2001
> From: Stephan Hilb
> Date: Sun, 10 Jun 2018 21:07:52 +0200
> Subject: [PATCH] lavd/v4l2: skip buffers not matching frame_size
>
> By adopting the same behaviour as if there was corrupted data in the
> buffer (see the check for V4L2_BUF_FLAG_ERROR) the resulting rawvideo
> now at least contains valid data (the previous frame being duplicated).
> Fixes video capturing for some stk1160 devices.
> ---
> libavdevice/v4l2.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c
> index 10a0ff0dd6..ab903bbcee 100644
> --- a/libavdevice/v4l2.c
> +++ b/libavdevice/v4l2.c
> @@ -534,11 +534,10 @@ static int mmap_read_frame(AVFormatContext *ctx,
> AVPacket *pkt)
> s->frame_size = buf.bytesused;
>
> if (s->frame_size > 0 && buf.bytesused != s->frame_size) {
> -av_log(ctx, AV_LOG_ERROR,
> +av_log(ctx, AV_LOG_WARNING,
> "Dequeued v4l2 buffer contains %d bytes, but %d were
> expected. Flags: 0x%08X.\n",
> buf.bytesused, s->frame_size, buf.flags);
> -enqueue_buffer(s, &buf);
> -return AVERROR_INVALIDDATA;
> +buf.bytesused = 0;
> }
> }
It is exactly the same as mine except for degrading the log message
from AV_LOG_ERROR to AV_LOG_WARNING which should be fine.
As I stated before I am in favor of this solution and I am fine with
applying your version. Let's wait a little while longer if there are
objections.
Alexander
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] avcodec/ccaption_dec: Add a blank like at the end to avoid rollup reading from outside
Hi Michael!
On 2019-04-20 18:11 +0200, Michael Niedermayer wrote:
> Fixes: index 20 out of bounds for type 'const char *[4][128]'
Somehow I don't understand this diagnostic message. It says "index
20 out of bounds for [4][128]".
You're change looks like an off-by-one fix.
Sorry; I'm surely missing something as I didn't look up the code,
just thought I might as well ask...
Alexander
> Fixes:
> 14367/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CCAPTION_fuzzer-5718819672162304
>
> 14367/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CCAPTION_fuzzer-5718819672162304
> Signed-off-by: Michael Niedermayer
> ---
> libavcodec/ccaption_dec.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/libavcodec/ccaption_dec.c b/libavcodec/ccaption_dec.c
> index 09ceb1b3bf..bf3563a0bc 100644
> --- a/libavcodec/ccaption_dec.c
> +++ b/libavcodec/ccaption_dec.c
> @@ -212,10 +212,10 @@ static const unsigned char pac2_attribs[32][3] = //
> Color, font, ident
>
> struct Screen {
> /* +1 is used to compensate null character of string */
> -uint8_t characters[SCREEN_ROWS][SCREEN_COLUMNS+1];
> -uint8_t charsets[SCREEN_ROWS][SCREEN_COLUMNS+1];
> -uint8_t colors[SCREEN_ROWS][SCREEN_COLUMNS+1];
> -uint8_t fonts[SCREEN_ROWS][SCREEN_COLUMNS+1];
> +uint8_t characters[SCREEN_ROWS+1][SCREEN_COLUMNS+1];
> +uint8_t charsets[SCREEN_ROWS+1][SCREEN_COLUMNS+1];
> +uint8_t colors[SCREEN_ROWS+1][SCREEN_COLUMNS+1];
> +uint8_t fonts[SCREEN_ROWS+1][SCREEN_COLUMNS+1];
> /*
> * Bitmask of used rows; if a bit is not set, the
> * corresponding row is not used.
> --
> 2.21.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] avfilter/af_astats: count number of NaNs/Infs/denormals for floating-point audio too
Hi Paul,
just three small comments from me...
On 2019-04-22 11:51 +0200, Paul B Mahol wrote:
> Signed-off-by: Paul B Mahol
> ---
> doc/filters.texi| 6 +++
> libavfilter/af_astats.c | 86 ++---
> 2 files changed, 86 insertions(+), 6 deletions(-)
I was a bit surprised when I first saw the number of lines it
takes to add this feature. OTOH this is not a problem of this
patch, because it is mostly caused by the structure of the
code that was in place before.
Changing the structure doesn't seem worth it yet. If
there will be an addition that is applicable to all the
individual stats, it should IMHO be reconsidered.
> diff --git a/doc/filters.texi b/doc/filters.texi
> index cfff9b1f4d..945c557e8f 100644
> --- a/doc/filters.texi
> +++ b/doc/filters.texi
> @@ -2141,6 +2141,9 @@ Bit_depth
> Dynamic_range
> Zero_crossings
> Zero_crossings_rate
> +Number_of_NaNs
> +Number_of_Infs
> +Number_of_denormals
Nit: Maybe Number_of_NaNs and Number_of_Infs should not be
capitalized like that, e.g. just use Number_of_nans and
Number_of_infs . Anyway if there's a reason for choosing
this names, it should be OK as it is. Just thinking it is
harder to type these from memory if the spelling is a bit
arbitrary.
> and for Overall:
> DC_offset
> @@ -2158,6 +2161,9 @@ Flat_factor
> Peak_count
> Bit_depth
> Number_of_samples
> +Number_of_NaNs
> +Number_of_Infs
> +Number_of_denormals
>
> For example full key look like this @code{lavfi.astats.1.DC_offset} or
> this @code{lavfi.astats.Overall.Peak_count}.
> diff --git a/libavfilter/af_astats.c b/libavfilter/af_astats.c
> index 92368793c2..25c700b344 100644
> --- a/libavfilter/af_astats.c
> +++ b/libavfilter/af_astats.c
> @@ -20,6 +20,7 @@
> */
>
> #include
> +#include
>
> #include "libavutil/opt.h"
> #include "audio.h"
> @@ -48,6 +49,9 @@
> #define MEASURE_ZERO_CROSSINGS (1 << 16)
> #define MEASURE_ZERO_CROSSINGS_RATE (1 << 17)
> #define MEASURE_NUMBER_OF_SAMPLES (1 << 18)
> +#define MEASURE_NUMBER_OF_NANS (1 << 19)
> +#define MEASURE_NUMBER_OF_INFS (1 << 20)
> +#define MEASURE_NUMBER_OF_DENORMALS (1 << 21)
>
> #define MEASURE_MINMAXPEAK (MEASURE_MIN_LEVEL |
> MEASURE_MAX_LEVEL | MEASURE_PEAK_LEVEL)
>
> @@ -68,6 +72,9 @@ typedef struct ChannelStats {
> uint64_t min_count, max_count;
> uint64_t zero_runs;
> uint64_t nb_samples;
> +uint64_t nb_nans;
> +uint64_t nb_infs;
> +uint64_t nb_denormals;
> } ChannelStats;
>
> typedef struct AudioStatsContext {
> @@ -83,6 +90,8 @@ typedef struct AudioStatsContext {
> int maxbitdepth;
> int measure_perchannel;
> int measure_overall;
> +int is_float;
> +int is_double;
> } AudioStatsContext;
>
> #define OFFSET(x) offsetof(AudioStatsContext, x)
> @@ -114,6 +123,9 @@ static const AVOption astats_options[] = {
>{ "Zero_crossings", "", 0, AV_OPT_TYPE_CONST,
> {.i64=MEASURE_ZERO_CROSSINGS }, 0, 0, FLAGS, "measure" },
>{ "Zero_crossings_rate" , "", 0, AV_OPT_TYPE_CONST,
> {.i64=MEASURE_ZERO_CROSSINGS_RATE }, 0, 0, FLAGS, "measure" },
>{ "Number_of_samples" , "", 0, AV_OPT_TYPE_CONST,
> {.i64=MEASURE_NUMBER_OF_SAMPLES }, 0, 0, FLAGS, "measure" },
> + { "Number_of_NaNs", "", 0, AV_OPT_TYPE_CONST,
> {.i64=MEASURE_NUMBER_OF_NANS }, 0, 0, FLAGS, "measure" },
> + { "Number_of_Infs", "", 0, AV_OPT_TYPE_CONST,
> {.i64=MEASURE_NUMBER_OF_INFS }, 0, 0, FLAGS, "measure" },
> + { "Number_of_denormals" , "", 0, AV_OPT_TYPE_CONST,
> {.i64=MEASURE_NUMBER_OF_DENORMALS }, 0, 0, FLAGS, "measure" },
> { "measure_overall", "only measure_perchannel these overall statistics",
> OFFSET(measure_overall), AV_OPT_TYPE_FLAGS, {.i64=MEASURE_ALL}, 0, UINT_MAX,
> FLAGS, "measure" },
> { NULL }
> };
> @@ -181,6 +193,9 @@ static void reset_stats(AudioStatsContext *s)
> p->max_count = 0;
> p->zero_runs = 0;
> p->nb_samples = 0;
> +p->nb_nans = 0;
> +p->nb_infs = 0;
> +p->nb_denormals = 0;
> }
> }
>
> @@ -196,6 +211,11 @@ static int config_output(AVFilterLink *outlink)
> s->tc_samples = 5 * s->time_constant * outlink->sample_rate + .5;
> s->nb_frames = 0;
> s->maxbitdepth = av_get_bytes_per_sample(outlink->format) * 8;
> +s->is_double = outlink->format == AV_SAMPLE_FMT_DBL ||
> + outlink->format == AV_SAMPLE_FMT_DBLP;
> +
> +s->is_float = outlink->format == AV_SAMPLE_FMT_FLT ||
> + outlink->format == AV_SAMPLE_FMT_FLTP;
>
> reset_stats(s);
>
> @@ -280,6 +300,24 @@ static inline void update_stat(AudioStatsContext *s,
> ChannelStats *p, double d,
> p->nb_samples++;
> }
>
> +static inline void update_float_stat(AudioStatsContext *s, ChannelStats *p,
> float d)
I guess "float d" is rather uncommon, but it's not unseen in the
FFmpeg code base.
> +{
> +
Re: [FFmpeg-devel] [PATCH V2 1/2] configure: sort decoder/encoder/filter/... names in alphabet order
Hi!
On 2019-04-15 21:23 +0800, Guo, Yejun wrote:
> take decoder names an example, with the default page length, shell command
> 'pr' needs two pages for all the decoder names. The names are firstly printed
> in the first page, then in the second page. So, as a whole, the names are
> sorted neither in column order nor in row order. It's a little confused.
>
> One method is to calculate the proper page length, so all the names are
> printed
> in one page by 'pr -l', and so strictly in alphabet order, column by column.
>
> Another method is to use command printf instead of pr, because buybox doesn't
> have pr. This patch refines print_in_columns to print the names with printf
> in alphabet order, very similar with 'pr -l', except the case when the last
> column is not fully filled with names.
Looks promising. Though this kind of change is basically
a bit difficult.
There is some risk that it won't work as expected on all
currently supported shells or perform differently.
For the performance as long as it is not grave, it should
not be a problem. Actually this implementation looks faster
here.
If it won't work as expected the risk is kind of limited,
as long as it doesn't modify global state that matters
and it does not abort the configure script, "only" the
informative / diagnostic output will be flawed, but the
build itself will be fine.
> Signed-off-by: Guo, Yejun
> ---
> configure | 34 +-
> 1 file changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/configure b/configure
> index c2580b3..45a9126 100755
> --- a/configure
> +++ b/configure
> @@ -3830,14 +3830,30 @@ die_unknown(){
> }
>
> print_in_columns() {
> -cols=$(expr $ncols / 24)
> -cat | tr ' ' '\n' | sort | pr -r "-$cols" -w $ncols -t
> +width=24
> +cols=$(expr $ncols / $width)
> +rows=$(expr $(expr $# + $cols - 1) / $cols)
> +eval format="%-${width}s"
Why do you use eval here?
> +for row in $(seq 1 $rows); do
I think the 1 argument to seq isn't needed here and below.
> +index=$row
> +line=""
> +lfmt=""
> +for col in $(seq 1 $cols); do
> +if [ $index -le $# ]; then
> +eval item=\$${index}
This will not work on all shells when trying to expand
the 10th parameter and following.
Excerpt of the relevant POSIX section:
When a positional parameter with more than one digit is specified, the
application shall enclose the digits in braces (see Parameter Expansion).
> +line=$line" "$item
> +lfmt=$lfmt$format
> +fi
> +index=$(expr $index + $rows)
> +done
> +printf "$lfmt\n" $line
> +done
The function will pad the rightmost column up to the column
width. It is usually not a visual problem in itself, but it
could lead to additional line breaks. Especially considering
that the typical values are significantly less than 24 chars.
> }
>
> show_list() {
> suffix=_$1
> shift
> -echo $* | sed s/$suffix//g | print_in_columns
> +print_in_columns $(echo $* | sed s/$suffix//g | tr ' ' '\n' | sort)
You are changing the interface of the print_in_columns:
1. passing input as paramters
2. making the sort external
Point 1 makes it a bit harder to read; preprocessing + sorting
pipelines are soaked into a command expansion that gets splitted
into the individual parameters.
Point 2 makes it more flexible, so it shouldn't be a problem.
If we decide to implement it this way, it might be better to
mention that change in the commit message explicitly.
[...]
I have experimented locally, inspired by your patch, I came
up with this version:
print_in_columns() {
col_width=24
cols=$(expr $ncols / $col_width)
rows=$(expr $(expr $# + $cols - 1) / $cols)
for idx in $(seq $rows); do
slice=
fmt=
for col in $(seq $cols); do
if test $idx -le $#; then
eval slice='"$slice "${'$idx'}'
fmt="${fmt}%-${col_width}s"
fi
idx=$(expr $idx + $rows)
done
printf "$fmt\n" $slice
done | sed 's/ *$//'
}
Tested with bash, dash and mksh.
Alexander
P.S.
Below follows the diff for a random configure command I tested here,
compared to the current implementation.
It's equivalent to the output of your implementation, minus the
trailing spaces after the last column. Speed is nearly the same.
diff --git a/configure-z.log b/configure-b.log
index e9a1834f4a..ca0bafdeef 100644
--- a/configure-z.log
+++ b/configure-b.log
@@ -60,157 +60,157 @@ Programs:
ffmpeg ffplay ffprobe
Enabled decoders:
-aacatrac1 eatqi
-aac_fixed atrac3 eightbps
-aac_latm atrac3aleightsvx_exp
-aasc atrac3p eightsvx_fib
-ac3atrac3pal escape124
-ac3_fixed
Re: [FFmpeg-devel] [PATCH] avfilter/af_astats: count number of NaNs/Infs/denormals for floating-point audio too
On 2019-04-25 18:49 +0200, Paul B Mahol wrote: > On 4/23/19, Alexander Strasser wrote: [...] > > > > On 2019-04-22 11:51 +0200, Paul B Mahol wrote: > >> Signed-off-by: Paul B Mahol > >> --- > >> doc/filters.texi| 6 +++ > >> libavfilter/af_astats.c | 86 ++--- > >> 2 files changed, 86 insertions(+), 6 deletions(-) > > > > I was a bit surprised when I first saw the number of lines it > > takes to add this feature. OTOH this is not a problem of this > > patch, because it is mostly caused by the structure of the > > code that was in place before. > > > > Changing the structure doesn't seem worth it yet. If > > there will be an addition that is applicable to all the > > individual stats, it should IMHO be reconsidered. > > What are you proposing to change? Sorry if I wasn't clear enough, but as I stated above I am not proposing to change things now. No objections against your patch! To clarify a bit what I meant with changing structure: There seem to be many places where things are duplicated for each individual statistic. Maybe things could be re-organized, so that it isn't needed to repeat those for every of the 2 dozen statistics that can be measured. But that's easier said than done, and even if one tried to do it, one could end up after a lot of work with a result that's not good enough :( That's why I said, maybe to reconsider it next time, when something gets changed that touches all statistics (if that will ever happen), because one would have to touch everything anyway and would probably be in good position (detailed mental model of the entire code of that filter) to try out structural changes. Pardon me, I'm having problems to express this more precisely at the moment. I hope the message got through. Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH V5 1/2] configure: sort decoder/encoder/filter/... names in alphabet order
Hi both of you,
nice work iterating on this patch set!
I want to discuss a little more, please don't take my comments
the wrong way, I just couldn't get back to this discussion earlier.
On 2019-04-24 13:36 +, Guo, Yejun wrote:
> >
> > From: avih [mailto:avih...@yahoo.com]
> > Sent: Wednesday, April 24, 2019 9:22 PM
> > To: FFmpeg development discussions and patches
> > Cc: Guo, Yejun
> > Subject: Re: [FFmpeg-devel] [PATCH V5 1/2] configure: sort
> > decoder/encoder/filter/... names in alphabet order
> >
> > > print_in_columns() {
> > > - cols=$(expr $ncols / 24)
> > > - cat | tr ' ' '\n' | sort | pr -r "-$cols" -w $ncols -t
> > > + # the input should not contain chars such as '*',
> > > + # otherwise, '*' will be expanded to be all files in the current
> > > + # working directory which don't begin with a dot (`.`)
> > > + set -- $(tr ' ' '\n' | sort)
> > > + col_width=24
> > > + if [ $ncols -lt $col_width ]; then
> > > + col_width=$ncols
> > > + fi
> > > + cols=$(($ncols / $col_width))
> > > + rows=$(($(($# + $cols - 1)) / $cols))
> > > + cols_seq=$(seq $cols)
> > > + rows_seq=$(seq $rows)
> > > + for row in $rows_seq; do
> > > + print_index=$row
> > > + print_line=""
> > > + for col in $cols_seq; do
> > > + if [ $print_index -le $# ]; then
> > > + eval print_line='"$print_line "${'$print_index'}'
> > > + fi
> > > + print_index=$(($print_index + $rows))
> > > + done
> > > + printf "%-${col_width}s" $print_line
> > > + printf "\n"
> > > + done | sed 's/ *$//'
> > > }
> >
> > Looks good to me. No further comments (but I don't push).
> >
> > Next time, know that you can use e.g. `$((x + y))` instead of `$(($x +
> > $y))`,
> > though in this case it doesn't matter and not worth another version.
>
> got it, thanks.
What do you think about using awk instead of shell?
I have 2 POC patches attached. It's probably not 100% correct yet,
but it kind of demonstrates what it would look like.
The main advantage of using awk, is that it's more elegant and
shorter. It's probably also less risky, because it's more isolated,
e.g. as it was explained by avih, there is no widely supported way
for locals across shells. We already use awk in configure for
mandatory functions, so it's no new dependency.
Please comment on the awk approach.
I'm not against the shell way, or a mixed approach, but before going
either way and pushing I would rather have some more testing;
especially on more exotic platforms.
Thank you
Alexander
From d04914411ec07cba29ae9b3cd43408f92c344958 Mon Sep 17 00:00:00 2001
From: Alexander Strasser
Date: Sat, 27 Apr 2019 23:15:08 +0200
Subject: [PATCH 1/2] configure: print_in_columns: Replace pr with awk
Get rid of pr dependency and write the columns strictly
alphabetical for the given rows.
Before pr would attempt to write pages, thus if a page
boundary was reached, the output looked confusing as one
couldn't see there was a new page and the alphabetical
order was disrupted when scanning down one of the columns.
Fixes part of ticket #TODO
---
configure | 20 ++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/configure b/configure
index b122b27268..7e739b3e55 100755
--- a/configure
+++ b/configure
@@ -3831,8 +3831,24 @@ die_unknown(){
}
print_in_columns() {
-cols=$(expr $ncols / 24)
-cat | tr ' ' '\n' | sort | pr -r "-$cols" -w $ncols -t
+sort | tr '\n' ' ' | awk -v width="$ncols" '
+{
+ num_cols = width / 24; num_rows = (NF + num_cols-1) / num_cols;
+ y = x = 1;
+for (i = 1; i <= NF; i++) {
+if (y > num_rows) {
+y = 1; x++;
+}
+d[x,y] = $i;
+y++;
+}
+for (y = 1; y <= num_rows; y++) {
+for (x = 1; x <= num_cols; x++) {
+line = sprintf(x != num_cols ? "%s%-24s" : "%s%s", line, d[x,y]);
+}
+print line; line = "";
+}
+}'
}
show_list() {
--
2.20.1
From 7dc4043db4ea32f6e85b17b6ec8060dac8c85f6b Mon Sep 17 00:00:00 2001
From: Alexander Strasser
Date: Sat, 27 Apr 2019 01:06:59 +0200
Subject: [PATCH 2/2] configure: log_file: Replace pr with awk invocation
Fixes remaining part of ticket #TODO
---
configure | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/configure b/configure
index 7e739b3e55..491d4d50e9 100755
--- a/configure
+++ b/configure
@@ -503,7 +503,7 @@ log(){
log_file(){
log BEGIN $1
-pr -n -t $1 >> $logfile
+awk '{ printf("%5d\t%s\n", NR, $0) }' $1 >> $logfile
log END $1
}
--
2.20.1
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH V5 1/2] configure: sort decoder/encoder/filter/... names in alphabet order
On 2019-04-28 03:11 +, avih wrote: > > What do you think about using awk instead of shell? > > No objection here, especially if it's more robust in some ways than this > or other shell code (though personally I'm not fluent in awk). > > My only concern was preventing a considerable performance impact which could > otherwise be avoided relatively easily, and with which I thought I could help. Yes, thank you. Very much appreciated! Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH V5 1/2] configure: sort decoder/encoder/filter/... names in alphabet order
On 2019-04-28 07:42 +, Guo, Yejun wrote:
> > -Original Message-
> > From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf Of
> > Alexander Strasser
> > Sent: Sunday, April 28, 2019 9:18 AM
> > To: FFmpeg development discussions and patches
> > Subject: Re: [FFmpeg-devel] [PATCH V5 1/2] configure: sort
> > decoder/encoder/filter/... names in alphabet order
> >
> >
> > What do you think about using awk instead of shell?
> >
> > I have 2 POC patches attached. It's probably not 100% correct yet,
> > but it kind of demonstrates what it would look like.
> >
> > The main advantage of using awk, is that it's more elegant and
> > shorter. It's probably also less risky, because it's more isolated,
> > e.g. as it was explained by avih, there is no widely supported way
> > for locals across shells. We already use awk in configure for
> > mandatory functions, so it's no new dependency.
> >
> > Please comment on the awk approach.
>
> I did some change to make it correct on ubuntu 16.04, but looks issue on
> windows mingw.
>
> print_in_columns() {
> sort | tr '\n' ' ' | awk -v width="$ncols" '
> {
> #add int()
> num_cols = int(width / 24); num_rows = int((NF + num_cols-1) /
> num_cols);
The added int() calls looks right.
> y = x = 1;
> for (i = 1; i <= NF; i++) {
> if (y > num_rows) {
> y = 1; x++;
> }
> d[x,y] = $i;
> y++;
> }
>
> print width
> print num_cols
> print num_rows
> print NF
>
> for (y = 1; y <= num_rows; y++) {
> for (x = 1; x <= num_cols; x++) {
> # it does not work for the case that the last column is not
> fully filled
> #line = sprintf(x != num_cols ? "%s%-24s" : "%s%s", line,
> d[x,y]);
> line = sprintf("%s%-24s", line, d[x,y]);
You mean it doesn't work in the way that there will be trailing spaces
in the column before the last one which is partially empty, right?
But the output looked right and there were never trailing spaces in the
last column, right?
BTW I changed the algorithm here, because I thought it might be easier
to unterstand. I will also try the algorithm, you implemented in shell.
> }
> print line;
> line = "";
> }
> }' | sed 's/ *$//'
> }
>
> on ubuntu 16.04, the output is:
> Enabled bsfs:
> 135
> 5
> 7
> 33
> aac_adtstoasc extract_extradata hevc_mp4toannexb
> mpeg4_unpack_bframestruehd_core
> av1_frame_split filter_unitsimx_dump_header noise
> vp9_metadata
> av1_metadatah264_metadata mjpeg2jpeg null
> vp9_raw_reorder
> chomp h264_mp4toannexbmjpega_dump_header
> prores_metadata vp9_superframe
> dca_coreh264_redundant_pps mov2textsub
> remove_extradatavp9_superframe_split
> dump_extradata hapqa_extract mp3_header_decompress
> text2movsub
> eac3_core hevc_metadata mpeg2_metadata
> trace_headers
>
> while on windows, the output is:
> Enabled bsfs:
> 72
> 3
> 11
> 33
> noiseh264_redundant_pps
> nullextract
> prores_metadata
>remove_extradatamp4toannexb
> text2movsubdump_header
> trace_headers
> truehd_corepega_dump_header
> vp9_metadata
> vp9_raw_reorderader_decompress
> vp9_superframea
>vp9_superframe_splitames
Wild guess: CR LF line endings are emitted somewhere and the CRs stay in
the input. Your terminal resets the cursor to the start of the line when
interpreting the midline CRs.
Does it work if you extend the tr in print_in_columns like below?
sort | tr '\r\n' ' ' | awk -v width="$ncols" '
> I did a quick check, but has not found the root cause yet.
>
>
> >
> > I'm not against the shell way, or a mixed approach, but before going
> > either way and pushing I would rather have some more testing;
> > especially on more exotic platforms.
Thanks for testing and looking into it!
Alexander
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 0/2] configure: Replace pr with awk and fix column sorting
Hi all, as was discussed in a previous patch set, this is a new try at getting the output of print_in_columns sorted in correct alphabetical order. The order with pr is messed up, because it assumes a page size and therefore breaks columns earlier. This is also a try to get rid of the pr dependency. There was a a wish for that in ticket #5680. I couldn't see a performance hit on my Linux and Windows systems with the first patch. For the second it depends on the used awk, where mawk and original awk were roughly in 1% margin, when using gawk it was around 6% slower. On Windows, it was once 8% slower on the first run and afterwards I could not reproduce the slowdown anymore. ATM I tend to prefer the v5 version from Yejun's patch set instead of the 2nd patch in this set. For me it's around 1.5% to 3% faster on Linux and about 5% on Windows, compared to the current version on master. I would appreciate more testing, especially on the less popular platforms. Alexander Strasser (2): configure: print_in_columns: Replace pr with awk configure: log_file: Replace pr with awk invocation configure | 20 +--- 1 file changed, 17 insertions(+), 3 deletions(-) -- ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 1/2] configure: print_in_columns: Replace pr with awk
Get rid of pr dependency and write the columns strictly
alphabetical for the given rows.
Before pr would attempt to write pages, thus if a page
boundary was reached, the output looked confusing as one
couldn't see there was a new page and the alphabetical
order was disrupted when scanning down one of the columns.
Fixes output for sizes with width < column width, too.
Fixes part of ticket #5680
Signed-off-by: Alexander Strasser
---
configure | 18 --
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/configure b/configure
index b122b27268..81e3776060 100755
--- a/configure
+++ b/configure
@@ -3831,8 +3831,22 @@ die_unknown(){
}
print_in_columns() {
-cols=$(expr $ncols / 24)
-cat | tr ' ' '\n' | sort | pr -r "-$cols" -w $ncols -t
+tr ' ' '\n' | sort | tr '\r\n' ' ' | awk -v col_width=24 -v
width="$ncols" '
+{
+num_cols = width > col_width ? int(width / col_width) : 1;
+num_rows = int((NF + num_cols-1) / num_cols);
+y = x = 1;
+for (y = 1; y <= num_rows; y++) {
+i = y;
+for (x = 1; x <= num_cols; x++) {
+if (i <= NF) {
+ line = sprintf("%s%-24s", line, $i);
+}
+i = i + num_rows;
+}
+print line; line = "";
+}
+}' | sed 's/ *$//'
}
show_list() {
--
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 2/2] configure: log_file: Replace pr with awk invocation
Fixes remaining part of ticket #5680
Signed-off-by: Alexander Strasser
---
configure | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/configure b/configure
index 81e3776060..96ada8f636 100755
--- a/configure
+++ b/configure
@@ -503,7 +503,7 @@ log(){
log_file(){
log BEGIN $1
-pr -n -t $1 >> $logfile
+awk '{ printf("%5d\t%s\n", NR, $0) }' $1 >> $logfile
log END $1
}
--
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH V5 1/2] configure: sort decoder/encoder/filter/... names in alphabet order
On 2019-04-29 00:55 +, Guo, Yejun wrote: [...] > > Wild guess: CR LF line endings are emitted somewhere and the CRs stay in > > the input. Your terminal resets the cursor to the start of the line when > > interpreting the midline CRs. > > > > Does it work if you extend the tr in print_in_columns like below? > > > > sort | tr '\r\n' ' ' | awk -v width="$ncols" ' > > > > yes, it works! > > > > > > I did a quick check, but has not found the root cause yet. > > > > > > > > > > > > > > I'm not against the shell way, or a mixed approach, but before going > > > > either way and pushing I would rather have some more testing; > > > > especially on more exotic platforms. > > > > Thanks for testing and looking into it! New patchset sent with subject: [PATCH 0/2] configure: Replace pr with awk and fix column sorting Your last version (v5) of the log_file rewrite performs better, so maybe we should use it instead of my awk version, which is at best as fast as current version (depends on the awk implementation). Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH V5 1/2] configure: sort decoder/encoder/filter/... names in alphabet order
Hi Avih! On 2019-05-02 08:55 +, avih wrote: > > It seems awk is unconditionally required already. However I wanted to > > say that it's a very nice dep to have > > While it's possibly nicer than other deps to have, it's still better to use > it IMHO only when it adds some value, like simpler code, better performance, > compliance with some things, etc. Agreed; of course we shouldn't just use awk because we can. Though I think not implementing things in shell is often lower risk, as we have no isolation in POSIX shell, we all share the same variables etc. and the configure script is quite big. Then shell is not suited for many tasks because of the way it works, like you explained really good before. So even if we find an acceptable solution, it will not be understood that easily by most people, because most people are not that deep into shell programming. Also in my experience the shells differ, when using more advanced concepts and in things that aren't used widely. Some of what I said applies to the alternatives too, except that they will always be better isolated. Sorry, that turned out longer than I wanted it to be :( > With this patchset, for part 1 I'm not sure I see the added value with awk, > as it looks about the same complexity of the code compared to the shell > version. Did you look at the version I attached in this thread? Or the one I posted in the new patch set? I changed it to use an algorithm more similar to the latest shell version discussed here. The version I attached in this mail thread, was an experiment to see if it could be easier understood that way. > Performance-wise it's negligible as print_in_columns() is only called > about 5 times (but with thousands of values to process), so as long as the > loop is efficient, the additional awk process per call is barely measurable. > For part 2 the awk version is simpler code, but `log_file()` is called nearly > 300 times, mostly with files with less than 5 lines to print. At this case it > does add a performance penalty, especially on systems where a new process is > expensive. Yes, exactly. Very well worded! Though I am still surprised about the Windows results. I did not investigate more, but I found it quite strange, that it showed a 8% performance hit the first time, and in subsequent runs there were no performance differences measurable for the runtime of the complete configure script. As mentioned in the other patch set, I am fine going with the shell version for part 2 (log_file). A third option would be to just use cat and forget about the line numbers. I am not sure the numbers really add much value, for those dumps of mostly very small source files with little ambiguity. Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH V5 1/2] configure: sort decoder/encoder/filter/... names in alphabet order
Hi Carl Eugen! On 2019-05-01 22:57 +0200, Carl Eugen Hoyos wrote: > 2019-04-28 3:18 GMT+02:00, Alexander Strasser : > > > What do you think about using awk instead of shell? > > Do we only use awk for --enable-random and the dependency > files so far? Does configure also work without awk now and > would this change? No, it wouldn't as I have written when initially proposing to use it here. We unconditionally use awk for generating and writing the primary configure outputs: the config files. Upon your question I looked it up exactly; the routine was rewritten by Mans to use awk near the end of 2012 in this commit: f454e879238ce317c6d905d187e7608c461a7087 Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH V5 2/2] configure: replace 'pr' with printf since busybox does not support pr
Hi all!
On 2019-04-28 00:38 +, Guo, Yejun wrote:
> > From: avih [mailto:avih...@yahoo.com]
> > Sent: Wednesday, April 24, 2019 9:23 PM
> > To: FFmpeg development discussions and patches
> > Cc: Guo, Yejun
> > Subject: Re: [FFmpeg-devel] [PATCH V5 2/2] configure: replace 'pr' with
> > printf
> > since busybox does not support pr
> >
> > > log_file(){
> > > - log BEGIN $1
> > > - pr -n -t $1 >> $logfile
> > > - log END $1
> > > + log BEGIN "$1"
> > > + log_file_i=1
> > > + while IFS= read -r log_file_line;do
> > > + printf '%5s %s\n' "${log_file_i}" "${log_file_line}"
I would like to do minimal adjustment to the line quoted above:
printf '%5d\t%s\n' "$log_file_i" "$log_file_line"
The \t makes the output equal to the current output. I would
prefer the %d because it makes the format a bit easier to grasp.
The removed {} pairs around log_file_i and log_file_line, aren't
needed and without them the style should be more consistent.
> > > + log_file_i=$(($log_file_i+1))
> > > + done < "$1" >> "$logfile"
> > > + log END "$1"
> > > }
> >
> > Looks good to me, no further comments (but I don't push).
>
> this patch set asks for push, or more comments, thanks.
It's faster than the current pr implementation.
If there are no objections to this patch in general and
to my suggested modifications in particular, I intent
to push it next week on friday.
Thanks
Alexander
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH V5 1/2] configure: sort decoder/encoder/filter/... names in alphabet order
Hi! On 2019-05-04 06:28 +, avih wrote: > > On 2019-05-02 08:55 +, avih wrote: > > > > It seems awk is unconditionally required already. However I wanted to > > > > say that it's a very nice dep to have > > > > > > While it's possibly nicer than other deps to have, it's still better to > > > use > > > it IMHO only when it adds some value, like simpler code, better > > > performance, > > > compliance with some things, etc. > > > > Agreed; of course we shouldn't just use awk because we can. > > > > Though I think not implementing things in shell is often > > lower risk, as we have no isolation in POSIX shell, we all > > share the same variables etc. and the configure script is > > quite big. Then shell is not suited for many tasks because > > of the way it works > > ... > > This actually sounds to me like you're saying we shouldn't use awk because > we can, but rather use it where possible because it'd be better than shell. > > In other words: we should write new configure code in awk. > > Did I misinterpret the statement or its implications? You got me totally wrong :( [...] > > > > Did you look at the version I attached in this thread? Or the one I > > posted in the new patch set? > > > > I changed it to use an algorithm more similar to the latest shell > > version discussed here. > > I think so, yes. As I said, it's similar to the shell version. I don't > think it's worse in any way, but I also didn't see an added value. > > Please post a link to the actual patch if you think I'm not looking > at the patch version you refer to. I guess you were looking at the right patch. I mean this one: http://ffmpeg.org/pipermail/ffmpeg-devel/2019-May/243380.html Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] configure: print_in_columns: Replace pr with awk
Am 5. Mai 2019 03:53:20 MESZ schrieb "Guo, Yejun" :
>
>
>> -Original Message-
>> From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf
>Of
>> Alexander Strasser
>> Sent: Thursday, May 02, 2019 12:08 AM
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: [FFmpeg-devel] [PATCH 1/2] configure: print_in_columns:
>Replace pr
>> with awk
>>
>> Get rid of pr dependency and write the columns strictly
>> alphabetical for the given rows.
>>
>> Before pr would attempt to write pages, thus if a page
>> boundary was reached, the output looked confusing as one
>> couldn't see there was a new page and the alphabetical
>> order was disrupted when scanning down one of the columns.
>>
>> Fixes output for sizes with width < column width, too.
>>
>> Fixes part of ticket #5680
>>
>> Signed-off-by: Alexander Strasser
>> ---
>> configure | 18 --
>> 1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/configure b/configure
>> index b122b27268..81e3776060 100755
>> --- a/configure
>> +++ b/configure
>> @@ -3831,8 +3831,22 @@ die_unknown(){
>> }
>>
>> print_in_columns() {
>> -cols=$(expr $ncols / 24)
>> -cat | tr ' ' '\n' | sort | pr -r "-$cols" -w $ncols -t
>> +tr ' ' '\n' | sort | tr '\r\n' ' ' | awk -v col_width=24 -v
>width="$ncols" '
>> +{
>> +num_cols = width > col_width ? int(width / col_width) : 1;
>> +num_rows = int((NF + num_cols-1) / num_cols);
>> +y = x = 1;
>> +for (y = 1; y <= num_rows; y++) {
>> +i = y;
>> +for (x = 1; x <= num_cols; x++) {
>> +if (i <= NF) {
>> + line = sprintf("%s%-24s", line, $i);
>
>not sure how to use col_width instead of the magic number 24.
Good catch! Fortunately it's easy to fix:
line = sprintf("%s%-" col_width "s", line, $i);
Will change it locally. Thanks.
Alexander
>> +}
>> +i = i + num_rows;
>> +}
>> +print line; line = "";
>> +}
>> +}' | sed 's/ *$//'
>> }
>>
>> show_list() {
>> --
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH V5 1/2] configure: sort decoder/encoder/filter/... names in alphabet order
On 2019-05-05 21:14 +, avih wrote: > > I guess you were looking at the right patch. I mean this one: > > http://ffmpeg.org/pipermail/ffmpeg-devel/2019-May/243380.html > > I was referring to this patch indeed. Thanks. > > > > > > Agreed; of course we shouldn't just use awk because we can. > > > > > > > > Though I think not implementing things in shell is often > > > > lower risk, as we have no isolation in POSIX shell, we all > > > > share the same variables etc. and the configure script is > > > > quite big. Then shell is not suited for many tasks because > > > > of the way it works > > > > ... > > > > > > This actually sounds to me like you're saying we shouldn't use awk because > > > we can, but rather use it where possible because it'd be better than > > > shell. > > > > > > In other words: we should write new configure code in awk. > > > > > > Did I misinterpret the statement or its implications? > > > > You got me totally wrong :( > > > I'm only human, it happens. But you didn't explain what you actually meant. It's a communication problem. I tried to explain why I prefer the awk version over the shell version of this patch in the email before. Seems I was too vague. I will try to answer your specific questions. If there is more to discuss, please don't hesitate to ask. > Specifically: > > - What makes this patch a good candidate to use awk rather than shell like > the rest of configure? I would like to rephrase this question a bit. I guess the other aspect will be in my answer to your next question anyway. patch1 (awk) configure: print_in_columns: replace pr with awk version: http://ffmpeg.org/pipermail/ffmpeg-devel/2019-May/243380.html patch2 (shell) configure: sort decoder/encoder/filter/... names in alphabet order (v5 as posted in this thread) - Why do you prefer patch1 over patch2? 1. Statistics * patch1: 1 file changed, 16 insertions(+), 2 deletions(-) * patch2: 1 file changed, 24 insertions(+), 2 deletions(-) 2. patch2 uses lots of variables (which is good in itself), but those should be local and we cannot express that portably in shell. In turn we have raised potential for clashes now or in the future. 3. patch2 uses eval combined with non-trivial quoting, which is hard to read and hard to grasp quickly. It's not wrong, but it's not as easy and straight forward as in patch1. 4. Depending on the input (stdin) unexpected things can happen in the "eval" and "set" lines. One example is given in the comment. 5. patch2 to uses shell for the parts easily expressed in shell and uses awk for the parts that are non-trivial in shell. 6. The awk implementation should be easier to read and understand for the majority of readers/developers. > - What should be the general criteria to choose a scripting language for > future patches? I don't see anywhere that we switch away from shell for configure. So in general things should be implemented in shell. Shell though is not really suited for implementing all kinds of algorithms. OTOH shell is an excellent choice for starting processes that communicate via stdin/stdout/stderr and plugging them together. So I think programming shell, sticking only to shell itself (builtins) is almost never a good option. Shell is best when coordinating the execution of other programs. Sometimes when you do not have a particular program around to help you in your shell script, that void needs to be filled. For this purpose awk is often a good choice, because it was especially designed to be a fit for this purpose amongst other things. Using only awk to implement bigger systems or as a general purpose programming language is IMHO nearly as wrong as using shell-only. > On Saturday, May 4, 2019 10:43 PM, Alexander Strasser > wrote: [...] I'm feeling like I might not have expressed my reasoning good and precise enough in every level of detail. I apologize for that. My natural language skills have their limits and I am not a native English speaker. I will try to refine specific points, should you or anyone else have more questions or comments. Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH V5 1/2] configure: sort decoder/encoder/filter/... names in alphabet order
Am 7. Mai 2019 04:07:23 MESZ schrieb "Guo, Yejun" :
>
>> From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf
>Of
>> Alexander Strasser
>> Sent: Tuesday, May 07, 2019 6:21 AM
>> To: FFmpeg development discussions and patches
>
>> Subject: Re: [FFmpeg-devel] [PATCH V5 1/2] configure: sort
>> decoder/encoder/filter/... names in alphabet order
>>
>> On 2019-05-05 21:14 +, avih wrote:
>> > > I guess you were looking at the right patch. I mean this one:
[...]
>> > > >
>> > > > In other words: we should write new configure code in awk.
>> > > >
>> > > > Did I misinterpret the statement or its implications?
>> > >
>> > > You got me totally wrong :(
>> >
>> > I'm only human, it happens. But you didn't explain what you
>actually meant.
>>
>> It's a communication problem. I tried to explain why I prefer
>> the awk version over the shell version of this patch in the email
>> before. Seems I was too vague.
>>
>> I will try to answer your specific questions. If there is more
>> to discuss, please don't hesitate to ask.
>>
>> > Specifically:
>> >
>> > - What makes this patch a good candidate to use awk rather than
>shell like
>> > the rest of configure?
>>
>> I would like to rephrase this question a bit. I guess the other
>aspect
>> will be in my answer to your next question anyway.
>>
>> patch1 (awk) configure: print_in_columns: replace pr with awk
>version:
>> http://ffmpeg.org/pipermail/ffmpeg-devel/2019-May/243380.html
>> patch2 (shell) configure: sort decoder/encoder/filter/... names in
>alphabet
>> order (v5 as posted in this thread)
>>
>> - Why do you prefer patch1 over patch2?
>>
>> 1. Statistics
>> * patch1: 1 file changed, 16 insertions(+), 2 deletions(-)
>> * patch2: 1 file changed, 24 insertions(+), 2 deletions(-)
>
>I have no idea which one is better, but I think this point is not the
>reason.
My fault. I wanted to add that those points are in no particular order and that
this one is not a very strong Point; but it's an indication.
>we can refine the shell patch as below with 15 insertions(+), 2
>deletions(-1). Actually, the line number does not mean something. :)
>
>print_in_columns() {
># the input should not contain chars such as '*', otherwise it will be
>expanded.
>set -- $(tr ' ' '\n' | sort)
>test $ncols -lt 24 && col_width=$ncols || col_width=24
>cols=$(($ncols / $col_width))
>rows=$(($(($# + $cols - 1)) / $cols))
>cols_seq=$(seq $cols)
>for row in $(seq $rows); do
>print_index=$row
>print_line=""
>for col in $cols_seq; do
>test $print_index -le $# && eval print_line='"$print_line
>"${'$print_index'}'
>print_index=$(($print_index + $rows))
>done
>printf "%-${col_width}s" $print_line; printf "\n"
>done | sed 's/ *$//'
>}
>
>the usage of "test && ||" is already used in configure.
Lines can bei sqeezed, but that isn't a very good Idea beyond some point. I'm
quite sure the shell Implementation will end up with a few lines more or I will
not feel comfortable pushing a patch that has been compacted too much.
As I said above already, that alone isn't a strong reason to prefer patch2.
Though all points together are in my opinion.
Best regards,
Alexander
>> 2. patch2 uses lots of variables (which is good in itself), but those
>>should be local and we cannot express that portably in shell.
>>In turn we have raised potential for clashes now or in the future.
>> 3. patch2 uses eval combined with non-trivial quoting, which is hard
>>to read and hard to grasp quickly. It's not wrong, but it's not as
>>easy and straight forward as in patch1.
>> 4. Depending on the input (stdin) unexpected things can happen in the
>>"eval" and "set" lines. One example is given in the comment.
>> 5. patch2 to uses shell for the parts easily expressed in shell and
>>uses awk for the parts that are non-trivial in shell.
>> 6. The awk implementation should be easier to read and understand for
>>the majority of readers/developers.
>>
>>
>> > - What should be the general criteria to choose a scripting
>language for
>> > future patches?
>>
>> I don't see anywhere that we switch away from shell for configure. So
Re: [FFmpeg-devel] [PATCH V5 2/2] configure: replace 'pr' with printf since busybox does not support pr
On 2019-05-05 01:38 +, Guo, Yejun wrote:
>
> > -Original Message-
> > Alexander Strasser
> > Sent: Sunday, May 05, 2019 3:42 AM
> >
> > On 2019-04-28 00:38 +, Guo, Yejun wrote:
> > > > From: avih [mailto:avih...@yahoo.com]
> > > > Sent: Wednesday, April 24, 2019 9:23 PM
> > > > To: FFmpeg development discussions and patches
> >
> > > > Cc: Guo, Yejun
> > > > Subject: Re: [FFmpeg-devel] [PATCH V5 2/2] configure: replace 'pr' with
> > printf
> > > > since busybox does not support pr
> > > >
> > > > > log_file(){
> > > > > - log BEGIN $1
> > > > > - pr -n -t $1 >> $logfile
> > > > > - log END $1
> > > > > + log BEGIN "$1"
> > > > > + log_file_i=1
> > > > > + while IFS= read -r log_file_line;do
> >
> >
> > > > > + printf '%5s %s\n' "${log_file_i}" "${log_file_line}"
> >
> > I would like to do minimal adjustment to the line quoted above:
> >
> > printf '%5d\t%s\n' "$log_file_i" "$log_file_line"
>
> It's good.
>
> >
> > The \t makes the output equal to the current output. I would
> > prefer the %d because it makes the format a bit easier to grasp.
> >
> > The removed {} pairs around log_file_i and log_file_line, aren't
> > needed and without them the style should be more consistent.
> >
> >
> > > > > + log_file_i=$(($log_file_i+1))
> > > > > + done < "$1" >> "$logfile"
> > > > > + log END "$1"
> > > > > }
> > > >
> > > > Looks good to me, no further comments (but I don't push).
> > >
> > > this patch set asks for push, or more comments, thanks.
> >
> > It's faster than the current pr implementation.
> >
> > If there are no objections to this patch in general and
> > to my suggested modifications in particular, I intent
> > to push it next week on friday.
Pushed.
Thanks to Yejun and Avi.
Alexander
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 2/2] configure: log_file: Replace pr with awk invocation
Dropped.
Pushed Yejun's version instead:
[PATCH V5 2/2] configure: replace 'pr' with printf since busybox does not
support pr
On 2019-05-01 18:08 +0200, Alexander Strasser wrote:
> Fixes remaining part of ticket #5680
>
> Signed-off-by: Alexander Strasser
> ---
> configure | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/configure b/configure
> index 81e3776060..96ada8f636 100755
> --- a/configure
> +++ b/configure
> @@ -503,7 +503,7 @@ log(){
>
> log_file(){
> log BEGIN $1
> -pr -n -t $1 >> $logfile
> +awk '{ printf("%5d\t%s\n", NR, $0) }' $1 >> $logfile
> log END $1
> }
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [DECISION] Ban Nicolas George from project
On 2019-05-18 11:57 -0300, James Almer wrote: > On 5/18/2019 9:24 AM, Paul B Mahol wrote: > > On 5/18/19, Reimar Döffinger wrote: > >> Voting is not yet another mobbing tool for when you get tired of other > >> ways. > >> If you wish for an end to that feud, you can ask other developers to try > >> and > >> help, but you've kept this going for years and usually responded somewhere > >> between dismissive and insulting to any such attempts. > >> If you wish to keep the feud going, please deal with the consequences > >> yourself and stop dragging the whole project into your self-created messes. > >> > >> Sorry for the bluntness and my excuses if I ruined the chances for a > >> peaceful weekend. > > > > You are obviously biased toward Nicolas, and far from being neutral. > > So I will just ignore your tries to be "helpful". > > You're not helping your cause in the slightest with this kind of reply. > > Just drop this. This is not how you solve a conflict between developers. > A call for a vote without first presenting the issue for debate to the > community isn't going anywhere. This is so sad :( And I am looking at both of you Nicolas and Paul. From what I know libavfilter wouldn't be anywhere near where it is today without you two! There needs to be room for discussion on the mailing list. If Nicolas has a comment that "blocks" one of your patches Paul, you should be polite and try to implement it or to find convincing argument that shows that it is better to do things differently. Yes, the process can be weary and demotivating at times. OTOH Nicolas should accept that often perfect is the enemy of good and having a solution now can be better than having it later or never. Also I think it is required to provide help and pointers and not only to say it is blocked because of X. Just less black and white. The world isn't ideal and there are advantages and drawbacks to each and every thing. Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] avfilter/f_loop: do not loop if loop size is 0
Hi!
On 2019-05-20 20:51 +0200, Marton Balint wrote:
>
> On Mon, 20 May 2019, Gyan wrote:
>
> > On 20-05-2019 02:18 AM, Marton Balint wrote:
> > >
> > > On Sun, 19 May 2019, Paul B Mahol wrote:
> > >
> > > > On 5/19/19, Marton Balint wrote:
> > > > >
> > > > > On Sun, 19 May 2019, Paul B Mahol wrote:
> > > > >
> > > > > > On 5/19/19, Marton Balint wrote:
> > > > > > > Fixes infinte loop with -vf loop=loop=1.
> > > > > > >
> > > > > > > Possible regression since
> > > > > > > ef1aadffc785b48ed62c45d954289e754f43ef46.
> > > > > > >
> > > > > > > Signed-off-by: Marton Balint
> > > > > > > ---
> > > > > > > libavfilter/f_loop.c | 2 +-
> > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/libavfilter/f_loop.c b/libavfilter/f_loop.c
> > > > > > > index d9d55f9837..3da753dd1e 100644
> > > > > > > --- a/libavfilter/f_loop.c
> > > > > > > +++ b/libavfilter/f_loop.c
> > > > > > > @@ -343,7 +343,7 @@ static int activate(AVFilterContext *ctx)
> > > > > > >
> > > > > > > FF_FILTER_FORWARD_STATUS_BACK(outlink, inlink);
> > > > > > >
> > > > > > > - if (!s->eof && (s->nb_frames < s->size || !s->loop)) {
> > > > > > > + if (!s->eof && (s->nb_frames < s->size ||
> > > > > > > !s->loop || !s->size)) {
> > > > > > > ret = ff_inlink_consume_frame(inlink, &frame);
> > > > > > > if (ret < 0)
> > > > > > > return ret;
> > > > > > > --
> > > > > >
> > > > > > I think better fix is to change default and minimal
> > > > > > allowed loop size to
> > > > > > 1.
> > > > > > Does that sounds ok to you?
> > > > >
> > > > > Well, looping the whole length of the input would be more
> > > > > intuitive to me
> > > > > as the default.
> > > >
> > > > That would require infinite memory.
> > >
> > > So as the reverse filter. As long as it is properly documented that
> > > the looped stuff is kept in memory so the user should not use this
> > > for long clips, then I think it is fine.
> >
> > I disagree. Yes, for loop with only loop specified, it would be
> > intuitive to loop the whole stream, but relying on users to exercise due
> > diligence can't be counted upon. We're talking about a scenario where
> > the user hasn't bothered to specify the size variable because they don't
> > know or don't care or are sloppy. They won't take heed of the
> > documentation until the command fails. The defaults should be robust
> > against lax use.
>
> Fair enough, although I never liked the idea that we make the tool less
> handy because we target unexperienced users.
FWIW, I guess the default behaviour of looping the complete input is much
better from a user perspective.
The typical users that have a need to loop a small clip will probably not
want to spefify a size in frames and will probably not really understand
why they need to specify one.
The typical users that want to loop a particular number of frames,
potentially at given offset into the specified input will probably read
the manual and in turn quickly find and use the size and/or start
options.
> Anyway, I don't have strong feelings about this, maybe my patch has the
> benefit of keeping existing behaviour (which is similar to how aloop works)
> in contrast to what paul suggested, but I don't really mind Paul's or Bela's
> solution either.
I have no strong feelings either, but it seems the behaviour
implemented by your patch seems ato fit more into the overall
situation too.
Alexander
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH V5 1/2] configure: sort decoder/encoder/filter/... names in alphabet order
Hi all, hi Avi! Sorry for the late reply. On 2019-05-07 12:22 +, avih wrote: > > patch1 (awk) configure: print_in_columns: replace pr with awk version: > > http://ffmpeg.org/pipermail/ffmpeg-devel/2019-May/243380.html > > patch2 (shell) configure: sort decoder/encoder/filter/... names in alphabet > > order (v5 as posted in this thread) > > > > - Why do you prefer patch1 over patch2? > > In general, I agree with your reasoning, but not entirely sure about the > conclusion and its implications. > > Few exceptions first: > > > > 1. Statistics > > * patch1: 1 file changed, 16 insertions(+), 2 deletions(-) > > * patch2: 1 file changed, 24 insertions(+), 2 deletions(-) > > As you said later, and I agree, it's a weak argument, as both versions > are the same code and complexity, but differ mostly in style and language > syntax. > > > 2. patch2 uses lots of variables (which is good in itself), but those > > should be local and we cannot express that portably in shell. > > In turn we have raised potential for clashes now or in the future. > > This is incorrect. A subshell provides identical isolation as an external > process, it's fully portable, it can be used here, and it's not slower than > an external process, e.g. `print_in_columns() ( ... )`. No disagreement regarding those 2 points. Though with all other things being equal, it's nice to have patches ready and tested as they are. > > 4. Depending on the input (stdin) unexpected things can happen in the > > "eval" and "set" lines. One example is given in the comment. > > "Unexpected" is a bit general. Specifically, the "eval" line does not > execute arbitrary inputs, and neither does the "set" line. Regarding eval it seems I misremembered something or it was in previous iteration. I looked at it again and I can't see how the latest version can do something unexpected trough the eval. > The only potential surprise is globbing, as documented, and the technique > of expanding variables unquoted - like here, is used throughout configure. > > Look no further than 100% of the very places which pipe the input stream > into print_in_columns, and multiple times earlier than that. > > It's not great of course, but it's not worse than elsewhere, and it's at > least documented. Some day maybe someone would improve it. > > > I generally agree with rest of your comments/arguments, including: > > > 1. Statistics ... (patch2 is more verbose). > > 3. patch2 uses eval combined with non-trivial quoting, which is hard > > to read and hard to grasp quickly. It's not wrong, but it's not as > > easy and straight forward as in patch1. > > 5. patch2 to uses shell for the parts easily expressed in shell and > > uses awk for the parts that are non-trivial in shell. > > 6. The awk implementation should be easier to read and understand for > > the majority of readers/developers. > > which basically say that shell is hard, and, pardon the pun, awkward. :) > And also I agree with: > > > Shell though is not really suited for implementing all kinds of algorithms. > > OTOH shell is an excellent choice for starting processes that communicate > > via stdin/stdout/stderr and plugging them together. > > and with > > > Shell is best when coordinating the execution of other programs. > > And similar assertions, however, let's keep the following facts in mind: > > - We both agree that we should keep to shell except where an external tool > provides some meaningful value over shell code. > > - An external tool only helps by being more suitable for the task than > shell code (readability, portability, correctness, performance, etc), > but it doesn't help with isolation more than a subshell would. > > - Shell is capable in a portable way, including for some tasks which can > be considered "programming". > > - configure is currently written in shell, including some non trivial > functionality, and doesn't use external script engines for "scriptlets". > > - Being a shell script, it already requires competence and reasonable > understanding of shell, including quoting, expansions, etc. > > - Maintaining code in more than one scripting system does carry a cost. > > - Yes, shell is a lousy programming language. > > > I really don't care if this specific patch ends up as shell or awk. What > I do care about is being consistent, and understanding the implications. > > IMHO, because I'm not convinced that the awk version provides a meaningful > additional value other than not being shell, if we use the awk version then > it sets a precedence of using awk (or maybe also other languages) for small > scripts even if they're basically the same complexity and code as they > would be in shell. > > It's not necessarily a bad precedence (even if I wouldn't do that myself), > because I do agree that not being shell code does have value, but I think > it's important to acknowledge that it does set such precedence. > > If there was already such practice of using awk script
[FFmpeg-devel] [PATCH] lavd/v4l2: produce a 0 byte packet when a dequeued buffer's size is unexpected
From: Stephan Hilb
Behave like we do for V4L2_BUF_FLAG_ERROR, implemented in commit 28f20d2ff4 .
For some devices (probably also related to the V4L driver implementation)
it happens that when invoking the ioctl DQBUF, the returned buffer is not
of the expected size. Here are two examples for such occurrences:
[video4linux2,v4l2 @ 0x258b440] Dequeued v4l2 buffer contains 609596 bytes,
but 614400 were expected. Flags: 0x0001.
/dev/video1: Invalid data found when processing input
[video4linux2,v4l2 @ 0x225f440] Dequeued v4l2 buffer contains 609508 bytes,
but 614400 were expected. Flags: 0x0001.
/dev/video1: Invalid data found when processing input
For the ffmpeg CLI tool this means it will stop capturing and exit.
The described behaviour was observed at least with one OmniVision USB
web cam and with some stk1160 devices.
If you search the web for the error message, you will find quite a few
instances of this problem. Some of them experienced on other devices.
Probably fixes ticket #4795
Signed-off-by: Alexander Strasser
---
This is exactly Stephan's patch except for the commit message.
@Stephan: I hope you are OK with my wording in the new message.
I contacted Giorgio off-list and also put him in Bcc for this email.
He previously reacted, but he probably doesn't have enough time. So if
there are no objections I intent to commit in roughly a week if no
more issues are found and no objections are raised.
libavdevice/v4l2.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c
index a9a0ed324d..446a243cf8 100644
--- a/libavdevice/v4l2.c
+++ b/libavdevice/v4l2.c
@@ -538,11 +538,10 @@ static int mmap_read_frame(AVFormatContext *ctx, AVPacket
*pkt)
s->frame_size = buf.bytesused;
if (s->frame_size > 0 && buf.bytesused != s->frame_size) {
-av_log(ctx, AV_LOG_ERROR,
+av_log(ctx, AV_LOG_WARNING,
"Dequeued v4l2 buffer contains %d bytes, but %d were
expected. Flags: 0x%08X.\n",
buf.bytesused, s->frame_size, buf.flags);
-enqueue_buffer(s, &buf);
-return AVERROR_INVALIDDATA;
+buf.bytesused = 0;
}
}
--
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH V5 1/2] configure: sort decoder/encoder/filter/... names in alphabet order
On 2019-06-03 06:24 +, Guo, Yejun wrote: > > > > -Original Message- > > From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf Of > > avih > > Sent: Monday, June 03, 2019 6:29 AM > > To: FFmpeg development discussions and patches > > Subject: Re: [FFmpeg-devel] [PATCH V5 1/2] configure: sort > > decoder/encoder/filter/... names in alphabet order > > > > > I intent to push the awk version. I will wait at least until > > > Thursday, so people can further test, comment, or object. > > > > No further comments here. > > I'm ok with the awk version. Finally pushed the awk version. Thanks again for your work and comments. Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavd/v4l2: produce a 0 byte packet when a dequeued buffer's size is unexpected
On 2019-06-05 22:04 +0200, Alexander Strasser wrote:
> From: Stephan Hilb
>
> Behave like we do for V4L2_BUF_FLAG_ERROR, implemented in commit 28f20d2ff4 .
>
> For some devices (probably also related to the V4L driver implementation)
> it happens that when invoking the ioctl DQBUF, the returned buffer is not
> of the expected size. Here are two examples for such occurrences:
>
> [video4linux2,v4l2 @ 0x258b440] Dequeued v4l2 buffer contains 609596
> bytes, but 614400 were expected. Flags: 0x0001.
> /dev/video1: Invalid data found when processing input
>
> [video4linux2,v4l2 @ 0x225f440] Dequeued v4l2 buffer contains 609508
> bytes, but 614400 were expected. Flags: 0x0001.
> /dev/video1: Invalid data found when processing input
>
> For the ffmpeg CLI tool this means it will stop capturing and exit.
>
> The described behaviour was observed at least with one OmniVision USB
> web cam and with some stk1160 devices.
>
> If you search the web for the error message, you will find quite a few
> instances of this problem. Some of them experienced on other devices.
>
> Probably fixes ticket #4795
>
> Signed-off-by: Alexander Strasser
> ---
>
> This is exactly Stephan's patch except for the commit message.
>
> @Stephan: I hope you are OK with my wording in the new message.
>
> I contacted Giorgio off-list and also put him in Bcc for this email.
>
> He previously reacted, but he probably doesn't have enough time. So if
> there are no objections I intent to commit in roughly a week if no
> more issues are found and no objections are raised.
Will push soon.
The current behavior surely causes problems for some users.
So it's better drop the short frame's data, log a warning
and have applications continue capturing instead of aborting.
It's also analog to how v4l2 indev behaves when it receives
corrupted data (V4L2_BUF_FLAG_ERROR).
Alexander
> libavdevice/v4l2.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c
> index a9a0ed324d..446a243cf8 100644
> --- a/libavdevice/v4l2.c
> +++ b/libavdevice/v4l2.c
> @@ -538,11 +538,10 @@ static int mmap_read_frame(AVFormatContext *ctx,
> AVPacket *pkt)
> s->frame_size = buf.bytesused;
>
> if (s->frame_size > 0 && buf.bytesused != s->frame_size) {
> -av_log(ctx, AV_LOG_ERROR,
> +av_log(ctx, AV_LOG_WARNING,
> "Dequeued v4l2 buffer contains %d bytes, but %d were
> expected. Flags: 0x%08X.\n",
> buf.bytesused, s->frame_size, buf.flags);
> -enqueue_buffer(s, &buf);
> -return AVERROR_INVALIDDATA;
> +buf.bytesused = 0;
> }
> }
>
> --
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] configure: print_in_columns: Replace pr with awk
On 2019-05-05 11:36 +0200, Alexander Strasser wrote:
>
>
> Am 5. Mai 2019 03:53:20 MESZ schrieb "Guo, Yejun" :
> >
> >
> >> -Original Message-
> >> From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf
> >Of
> >> Alexander Strasser
> >> Sent: Thursday, May 02, 2019 12:08 AM
> >> To: ffmpeg-devel@ffmpeg.org
> >> Subject: [FFmpeg-devel] [PATCH 1/2] configure: print_in_columns:
> >Replace pr
> >> with awk
> >>
> >> Get rid of pr dependency and write the columns strictly
> >> alphabetical for the given rows.
> >>
> >> Before pr would attempt to write pages, thus if a page
> >> boundary was reached, the output looked confusing as one
> >> couldn't see there was a new page and the alphabetical
> >> order was disrupted when scanning down one of the columns.
> >>
> >> Fixes output for sizes with width < column width, too.
> >>
> >> Fixes part of ticket #5680
> >>
> >> Signed-off-by: Alexander Strasser
> >> ---
> >> configure | 18 --
> >> 1 file changed, 16 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/configure b/configure
> >> index b122b27268..81e3776060 100755
> >> --- a/configure
> >> +++ b/configure
> >> @@ -3831,8 +3831,22 @@ die_unknown(){
> >> }
> >>
> >> print_in_columns() {
> >> -cols=$(expr $ncols / 24)
> >> -cat | tr ' ' '\n' | sort | pr -r "-$cols" -w $ncols -t
> >> +tr ' ' '\n' | sort | tr '\r\n' ' ' | awk -v col_width=24 -v
> >width="$ncols" '
> >> +{
> >> +num_cols = width > col_width ? int(width / col_width) : 1;
> >> +num_rows = int((NF + num_cols-1) / num_cols);
> >> +y = x = 1;
> >> +for (y = 1; y <= num_rows; y++) {
> >> +i = y;
> >> +for (x = 1; x <= num_cols; x++) {
> >> +if (i <= NF) {
> >> + line = sprintf("%s%-24s", line, $i);
> >
> >not sure how to use col_width instead of the magic number 24.
>
> Good catch! Fortunately it's easy to fix:
>
> line = sprintf("%s%-" col_width "s", line, $i);
>
> Will change it locally. Thanks.
Pushed with this change and a more elaborate commit message.
Alexander
> >> +}
> >> +i = i + num_rows;
> >> +}
> >> +print line; line = "";
> >> +}
> >> +}' | sed 's/ *$//'
> >> }
> >>
> >> show_list() {
> >> --
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavd/v4l2: produce a 0 byte packet when a dequeued buffer's size is unexpected
On 2019-06-25 00:08 +0200, Alexander Strasser wrote: > On 2019-06-05 22:04 +0200, Alexander Strasser wrote: > > From: Stephan Hilb > > > > Behave like we do for V4L2_BUF_FLAG_ERROR, implemented in commit 28f20d2ff4 > > . > > > > For some devices (probably also related to the V4L driver implementation) > > it happens that when invoking the ioctl DQBUF, the returned buffer is not > > of the expected size. Here are two examples for such occurrences: > > > > [video4linux2,v4l2 @ 0x258b440] Dequeued v4l2 buffer contains 609596 > > bytes, but 614400 were expected. Flags: 0x0001. > > /dev/video1: Invalid data found when processing input > > > > [video4linux2,v4l2 @ 0x225f440] Dequeued v4l2 buffer contains 609508 > > bytes, but 614400 were expected. Flags: 0x0001. > > /dev/video1: Invalid data found when processing input > > > > For the ffmpeg CLI tool this means it will stop capturing and exit. > > > > The described behaviour was observed at least with one OmniVision USB > > web cam and with some stk1160 devices. > > > > If you search the web for the error message, you will find quite a few > > instances of this problem. Some of them experienced on other devices. > > > > Probably fixes ticket #4795 > > > > Signed-off-by: Alexander Strasser > > --- > > > > This is exactly Stephan's patch except for the commit message. > > > > @Stephan: I hope you are OK with my wording in the new message. > > > > I contacted Giorgio off-list and also put him in Bcc for this email. > > > > He previously reacted, but he probably doesn't have enough time. So if > > there are no objections I intent to commit in roughly a week if no > > more issues are found and no objections are raised. > > Will push soon. Pushed. Not quite as soon as planned, but hopefully better late than never. [...] Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/3] avformat/avio: add avio_print_n_strings and avio_print
Hi Marton!
Not really sure if we need the API, but it definitely looks
handy. Just not sure how often it will used and really result
in clearer or shorter code.
Not opposed to it though; no strong opinion on this one.
Some comments follow inline. No in depth review, just what
came to my mind when reading your patch quickly.
On 2019-08-05 23:34 +0200, Marton Balint wrote:
> These functions can be used to print a variable number of strings
> consecutively
> to the IO context. Unlike av_bprintf, no temporery buffer is necessary.
Small typo here: temporary
> Signed-off-by: Marton Balint
> ---
> doc/APIchanges| 3 +++
> libavformat/avio.h| 16
> libavformat/aviobuf.c | 17 +
> libavformat/version.h | 2 +-
> 4 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 6603a8229e..0b19fb067d 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,9 @@ libavutil: 2017-10-21
>
> API changes, most recent first:
>
> +2019-08-xx - xx - lavf 58.31.100 - avio.h
> + Add avio_print_n_strings and avio_print.
> +
> 2019-07-27 - xx - lavu 56.33.100 - tx.h
>Add AV_TX_DOUBLE_FFT and AV_TX_DOUBLE_MDCT
>
> diff --git a/libavformat/avio.h b/libavformat/avio.h
> index dcb8dcdf93..ca08907917 100644
> --- a/libavformat/avio.h
> +++ b/libavformat/avio.h
> @@ -574,6 +574,22 @@ int avio_feof(AVIOContext *s);
> /** @warning Writes up to 4 KiB per call */
> int avio_printf(AVIOContext *s, const char *fmt, ...) av_printf_format(2, 3);
>
> +/**
> + * Write nb_strings number of strings (const char *) to the context.
> + * @return the total number of bytes written
> + */
> +int avio_print_n_strings(AVIOContext *s, int nb_strings, ...);
> +
> +/**
> + * Write strings (const char *) to the context.
> + * This is a macro around avio_print_n_strings but the number of strings to
> be
> + * written is determined automatically at compile time based on the number of
> + * parameters passed to this macro. For simple string concatenations this
> + * function is more performant than using avio_printf.
> + */
> +#define avio_print(s, ...) \
> +avio_print_n_strings(s, sizeof((const char*[]){__VA_ARGS__}) /
> sizeof(const char*), __VA_ARGS__)
If we don't have this pattern in the code base already, it would
be better to compile and test on bunch of different compilers.
> +
> /**
> * Force flushing of buffered data.
> *
> diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
> index 2d011027c9..c37b056b64 100644
> --- a/libavformat/aviobuf.c
> +++ b/libavformat/aviobuf.c
> @@ -1225,6 +1225,23 @@ int avio_printf(AVIOContext *s, const char *fmt, ...)
> return ret;
> }
>
> +int avio_print_n_strings(AVIOContext *s, int nb_strings, ...)
> +{
> +va_list ap;
> +int ret = 0;
> +
> +va_start(ap, nb_strings);
> +for (int i = 0; i < nb_strings; i++) {
> +const char *str = va_arg(ap, const char *);
> +int len = strlen(str);
> +avio_write(s, (const unsigned char *)str, len);
> +ret += len;
I first wanted to say you should compute with the count returned
by avio_write; then after diving into libavformat/avio_buf and
reading a cascade of void functions and seeing signalling via
field error in the context which also is sometimes (mis)used
to store a length(?), I withdraw that comment.
Anyway you might consider using void for this function too,
otherwise I guess you should figure out how to do the error
checking inside of this function because if an error occurs
that call might have been partial and the following writes will
effectively not occur. So I'm feeling rather uncomfortable
with returning a count here. Maybe my stance is to narrow.
Alexander
> +}
> +va_end(ap);
> +
> +return ret;
> +}
> +
> int avio_pause(AVIOContext *s, int pause)
> {
> if (!s->read_pause)
> diff --git a/libavformat/version.h b/libavformat/version.h
> index 45efaff9b9..feceaedc08 100644
> --- a/libavformat/version.h
> +++ b/libavformat/version.h
> @@ -32,7 +32,7 @@
> // Major bumping may affect Ticket5467, 5421, 5451(compatibility with
> Chromium)
> // Also please add any ticket numbers that you believe might be affected here
> #define LIBAVFORMAT_VERSION_MAJOR 58
> -#define LIBAVFORMAT_VERSION_MINOR 30
> +#define LIBAVFORMAT_VERSION_MINOR 31
> #define LIBAVFORMAT_VERSION_MICRO 100
>
> #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \
> --
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] doc/developper: always use braces for statements
On 2019-08-06 22:51 +0200, Nicolas George wrote: > Tomas Härdin (12019-08-06): > > I think they should. See > > Documenting the coding standard and changing it are different matters. I guess a change was intended, because it should be fairly obvious that in the current code base the described style for ifs with a single statement body is barely used. IMHO the only con is that it uses up more vertical space (one line more, when sticking with the brace placement as is) and thus less real code fits on a screen/page. Anyway despite of other things pointed out below. There are additional advantages like making it easier to debug and to extend the code with less changes. In turn making the potentially resulting patches shorter and easier to read. > > https://www.imperialviolet.org/2014/02/22/applebug.html > > test_warn.c: In function ‘test’: > test_warn.c:6:5: warning: this ‘if’ clause does not guard... > [-Wmisleading-indentation] > if (x == y) > ^~ > test_warn.c:8:9: note: ...this statement, but the latter is misleadingly > indented as if it were guarded by the ‘if’ > return -1; > ^~ After all Style is subjective, and thus of course you are right, Nicolas, changes to the coding style should be discussed and not be tunneled through documentation patches. Though I don't think the latter was the intention of the OP. Clearly communicating the request for change would have been better though. Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] libavcodec: add timer bitstream filter [v2]
Hi Andreas!
On 2019-08-05 17:25 +, Andreas Håkon wrote:
> On Monday, 5 de August de 2019 17:31, Moritz Barsnick
> wrote:
>
[...]
> > > +static const AVClass timer_class = {
> > > + .class_name = "timer",
> >
> > In about half of the existing BSFs, this would be called "timer_bsf". I
> > don't care, I just look at other existing code. ;-)
> >
>
> Good point! Other bitstream filters have this. I'll update it.
> Thank you!
Sorry for the noise. IIRC the naming was previously discussed.
Can't remember the details right now. Unfortunately I don't have
any good name suggestions too :(
This is no objections, but at least to me the name timer sounds
like some autonomous unit/process that can interrupt/call your
process on defined/configured/subscribed intervals. Maybe it's
just me...
...what do others think?
Alexander
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/3] avformat/avio: add avio_print_n_strings and avio_print
On 2019-08-07 21:28 +0200, Marton Balint wrote:
>
> On Wed, 7 Aug 2019, Alexander Strasser wrote:
>
> > Hi Marton!
> >
> > Not really sure if we need the API, but it definitely looks
> > handy. Just not sure how often it will used and really result
> > in clearer or shorter code.
>
> It has better performance than using av_printf because it does not need a
> temporary buffer.
True. I meant one could use avio_write like Paul demonstrated
or like Nicolas suggested introduce a simple wrapper function
avio_write_string .
But as I said right below, I'm not against this API.
> > Not opposed to it though; no strong opinion on this one.
> > Some comments follow inline. No in depth review, just what
> > came to my mind when reading your patch quickly.
> >
> >
> > On 2019-08-05 23:34 +0200, Marton Balint wrote:
> > > These functions can be used to print a variable number of strings
> > > consecutively
> > > to the IO context. Unlike av_bprintf, no temporery buffer is necessary.
> >
> > Small typo here: temporary
>
> Fixed locally.
>
> >
> > > Signed-off-by: Marton Balint
> > > ---
> > > doc/APIchanges| 3 +++
> > > libavformat/avio.h| 16
> > > libavformat/aviobuf.c | 17 +
> > > libavformat/version.h | 2 +-
> > > 4 files changed, 37 insertions(+), 1 deletion(-)
[...]
> > >
> > > +/**
> > > + * Write nb_strings number of strings (const char *) to the context.
> > > + * @return the total number of bytes written
> > > + */
> > > +int avio_print_n_strings(AVIOContext *s, int nb_strings, ...);
> > > +
> > > +/**
> > > + * Write strings (const char *) to the context.
> > > + * This is a macro around avio_print_n_strings but the number of strings
> > > to be
> > > + * written is determined automatically at compile time based on the
> > > number of
> > > + * parameters passed to this macro. For simple string concatenations this
> > > + * function is more performant than using avio_printf.
> > > + */
> > > +#define avio_print(s, ...) \
> > > +avio_print_n_strings(s, sizeof((const char*[]){__VA_ARGS__}) /
> > > sizeof(const char*), __VA_ARGS__)
> >
> > If we don't have this pattern in the code base already, it would
> > be better to compile and test on bunch of different compilers.
>
> I tested a few compilers (Clang, GCC, MSVC 2015) on https://godbolt.org/ and
> it seems to work.
Sounds good.
Another thing is, that the convenient macro will probably only be
usable in C client code. That's of course expected, and bindings
could provide similar implementations inspired by your design of
avio_print, which might be easier anyway in many cases. Just saying
because I think we should consider those things when designing and
extending FFmpeg's APIs.
[...]
> > >
> > > +int avio_print_n_strings(AVIOContext *s, int nb_strings, ...)
> > > +{
> > > +va_list ap;
> > > +int ret = 0;
> > > +
> > > +va_start(ap, nb_strings);
> > > +for (int i = 0; i < nb_strings; i++) {
> > > +const char *str = va_arg(ap, const char *);
> > > +int len = strlen(str);
> > > +avio_write(s, (const unsigned char *)str, len);
> > > +ret += len;
> >
> > I first wanted to say you should compute with the count returned
> > by avio_write; then after diving into libavformat/avio_buf and
> > reading a cascade of void functions and seeing signalling via
> > field error in the context which also is sometimes (mis)used
> > to store a length(?), I withdraw that comment.
> >
> > Anyway you might consider using void for this function too,
> > otherwise I guess you should figure out how to do the error
> > checking inside of this function because if an error occurs
> > that call might have been partial and the following writes will
> > effectively not occur. So I'm feeling rather uncomfortable
> > with returning a count here. Maybe my stance is to narrow.
>
> It returns int because avio_printf also returns int, but since no error
> (other than IO error) can happen, maybe it is better to return void the same
> way as avio_write functions do. For IO errors the user should always check
> the IO context error flag, that is by design as far as I know.
>
> I'll change it to return void.
I have seen it in your patch version 2. Looks fine.
No more comments from me.
Thanks,
Alexander
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] libavcodec: add timeshift bitstream filter [v3]
On 2019-08-07 15:51 +, Andreas Håkon wrote: > Hi, > > This new version changes the name of the filter, and implements all > suggestions. Sorry, I couldn't reply earlier. Thanks for renaming; "timeshift" sounds better to me, compared to the previous "timer". Other suggestions which I came up with: * adjust_timestamps * edit_timestamps Instead of the _timestamps suffix _ts or _ptsdts could be used. Maybe others have better suggestions. For me the current name timeshift is acceptable. If you decide to rename it yet again, I would recommend to not send new versions of the patch with only name change for now. It's better to wait for potential review comments. To make it clear I did only read your patch and *didn't review* it at all. I wanted to comment on the name in time, because it's always significantly more effort to deal with user visible naming once the code is merged. Alexander > Supersedes: > https://patchwork.ffmpeg.org/patch/14195/ > https://patchwork.ffmpeg.org/patch/13743/ > https://patchwork.ffmpeg.org/patch/13699/ > > Regards. > A.H. > > --- > From 9e036e0a1dc20fd5c41686a48a5a0c2142f91de4 Mon Sep 17 00:00:00 2001 > From: Andreas Hakon > Date: Wed, 7 Aug 2019 17:45:25 +0200 > Subject: [PATCH] libavcodec: add timeshift bitstream filter [v3] > > This implements the new timeshift bitstream filter. > > Signed-off-by: Andreas Hakon > --- > doc/bitstream_filters.texi | 11 + > libavcodec/Makefile|1 + > libavcodec/bitstream_filters.c |1 + > libavcodec/timeshift_bsf.c | 87 > > 4 files changed, 100 insertions(+) > create mode 100644 libavcodec/timeshift_bsf.c > [...] ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] libavcodec: add timeshift bitstream filter [v3]
Hi Nicolas! On 2019-08-08 11:43 +0200, Nicolas George wrote: > Andreas Håkon (12019-08-08): > > > setpts or setts, similar to lavfi. > > > Then I proposse these two alternatives: > > > > 1) "editpts_bsf" > > 2) "setpts_bsf" > > > > The "_bsf" is not real, only to differenciate (here) from other filters. > > Which one do you prefer? > > editpts has no similarity with lavfi. Users would need to remember if it > is filters <-> set and bsf <-> edit or filters <-> edit and bsf <-> set. > Better have a single name. Right, I thought about that too. It could get confusing. Still my initial suggestions were on purpose not setpts. More on that below. > Bitstream filters and lavfi are not in the same namespace, therefore the > suffix is unneeded. > > > Gyan (12019-08-08): > > This BSF only allows to apply a fixed offset whereas the setpts filter > > allows arbitrary expressions. Unless someone is planning to extend this > > BSF, the current candidate is more apt. Like Gyan mentioned here, that was my reason to refrain from proposing setpts as a name for this bsf. I too think, that one would expect being able to use free expressions for the timestamp specification. And that would cause similar confusions like the one you described above, where users would think "ah, it's setpts I can just give it the offset I want" vs "ah, it's setpts I can give it an expressions that evaluates to the time stamp I want to set" Also if I'm not mistaken for the lavfi filter the name setpts is on spot whereas for the bsf we would also deal with dts, which was not a consideration when implementing the setpts filter and defining its arguments, which is just a single argument named "expr". > At some point, somebody will implement generic expression eval. That might happen, but it's IMHO too vague to rely on for making name decisions right in this moment. > When > that happens, it will be better if they can just add an option to the > existing filter rather than creating a whole new filter, deprecating the > existing one, etc. That sounds a bit exaggerating to me: 1. Building a whole new bsf would not be that different from changing the existing one, except from having to copy and modify the boiler plate related to implementing bsfs in general. Maybe a couple of lines in the existing implementation of the bsf could be reused, maybe not. 2. The existing one wouldn't have to be deprecating, as it's so simple that it could stay there providing an alternative filter with a different interface and implementation. Or the code could be shared, which at the moment doesn't seem to have any benefits, but would also share the flaws for which there will be more opportunities in the bsf with expression evaluation. 3. If one would decide, one doesn't want to have the two achieving the same thing. One could replace the documentation of this bsf with "Only for backward compatibility. Use bsf setpts." So yes it might be better if, but even then probably not much. Also if it happens that all set filters of lavfi really get merged into a single filter, we would have chosen the wrong name now. > Key idea: long term planning. I agree that long term planning is important, though I disagree with your conclusion in this specific discussion. The whole bsf framework is probably a nice long-term move. It's flexibility of allowing bsfs as individual filters that are handled in a generic fashion, provides a good ground for implementing a diverse, useful and partially redundant set of bsfs. Seeing it this way it eases long-term planning by removing the need for detailed planning and allowing experimentation and evolution without having to sacrifice compatibility. > Therefore, I am for setpts (dts is just secondary), with a mention in > the documentation: > > # Unlike the setpts filter in libavfilter, this filter currently can > # only apply a constant offset. Another name idea * ts_offset loosely in reference to -itsoffset and output_ts_offset (AVFormatContext). And from the initial discussion there was: * add_delay Regarding setpts and other filters like addroi and so on, I find it a bit unfortunate that they don't have an underscore. "set_" and "add_" is much better for doing full text searches on the docs or the complete codebase. If you still think setpts is the best name for that bsf, it's fine for me; my main concern was to not name it "timer". Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] avutil: Add NV61 pixel format and enable IO support for NV16 and NV61 pixel formats
On 2020-06-19 20:41 +0200, Carl Eugen Hoyos wrote: > Am Fr., 19. Juni 2020 um 20:33 Uhr schrieb Cristian Bicheru > : > > > > On Fri, Jun 19, 2020 at 12:53 PM Carl Eugen Hoyos > > wrote: > > > > > > Am Fr., 19. Juni 2020 um 18:42 Uhr schrieb : > > > > > > > > From: Cristian Bicheru > > > > > > > > NV16 and NV61 are interleaved 4:2:2 8-bit formats. There was some > > > > internal > > > > NV16 support prior to this patch but the pixel format was not publicly > > > > accessible. > > > > Please explain (as thorough as possible) why the new pix_fmt is necessay. > > > > I mainly added the new format for completion > > This does not sound like a good reason to add complexity. > > If you find a camera that only supports NV61, I would say > we should reconsider. I guess NV61 is not super important, but it's kind of an interesting case to use for reasoning about if additional pixel formats may be added to the code base or not. For example "If you find a camera that only supports NV61" seems a bit to narrow as a guideline. I think if there is equipement that supports both variants NV16 and NV61, that could be enough to support NV61 too. One could also argue adding a well defined pixel format should be fine especially if it is easy. In this case it is a simple and low complexity change, as we already support NV16 anyway. How do others think about adding support for more pixel formats? Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [Patch] vf_edgedetect: properly implement double_threshold()
Hi Valery! On 2020-06-19 17:15 +0200, Valery Kot wrote: > vf_edgedetect video filter implements Canny algorithm > (https://en.wikipedia.org/wiki/Canny_edge_detector) > > Important part of this algo is the double threshold step: pixels above > "high" threshold being kept, pixels below "low" threshold dropped, > pixels in between kept if they are attached to "high" pixels. > > This is implemented in the double_threshold() function. However, > condition to start checking attached pixels, as it is now and as it > was in FFmpeg since 2012, only allows checking on the boundary, not > inside the video. It is a very lucky coincidence that those boundary > pixels are always 0, otherwise following lines would be reading > outside of the buffer. > > As it is now, double_threshold() only implements "high" thresholding. > As a result, edges are either noisy or fragmented, depending on "high" > threshold selection; "low" threshold is simply ignored. > > Attached one char patch fixes this. > > Please review. Looks indeed like the condition is wrong. I say looks because I did only look and didn't do actual testing. I hope you will answer my question anyway: Does your patch completely fix the problem or is it sacrificing the treatment of the outer most pixels? Alexander > From b78f5960736de52d1c8e41bd598a465092c1de60 Mon Sep 17 00:00:00 2001 > From: vkot > Date: Fri, 19 Jun 2020 16:57:13 +0200 > Subject: [PATCH] vf_edgedetect: properly implement double_threshold() > > --- > libavfilter/vf_edgedetect.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavfilter/vf_edgedetect.c b/libavfilter/vf_edgedetect.c > index a5614ea63b..df8afbd532 100644 > --- a/libavfilter/vf_edgedetect.c > +++ b/libavfilter/vf_edgedetect.c > @@ -294,7 +294,7 @@ static void double_threshold(int low, int high, int w, > int h, > continue; > } > > -if ((!i || i == w - 1 || !j || j == h - 1) && > +if (!(!i || i == w - 1 || !j || j == h - 1) && > src[i] > low && > (src[-src_linesize + i-1] > high || > src[-src_linesize + i ] > high || > -- ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] avutil: Add NV61 pixel format and enable IO support for NV16 and NV61 pixel formats
Am 20. Juni 2020 00:23:53 MESZ schrieb Hendrik Leppkes : >On Fri, Jun 19, 2020 at 9:58 PM Alexander Strasser >wrote: >> >> How do others think about adding support for more pixel formats? >> > >A new pixel format should present a clear improvement, a use-case you >couldn't do before, or could only do with a performance penalty or >whatever. Thanks for your quick reply. Generally I agree here. >If everything that you can do with NV61 you could also do with NV16, a >format we already have, which value does adding it add to the project In this case it's true, that you can't do anything special with NV61 you can't do with NV16, but I think they can also be viewed as the same format. So if you add one, you also add the other. Like when I would add NV12 support I would also add NV21 support. It has the added value, that whenever a user needs one or the other as input or output it would be available. On the implementation side it is kind of easy to test, that both work the same with chroma interleaving swapped. But maybe I misunderstand something important here? >Completionism should not be a goal. There are hundreds of obscure >pixel formats that we have no business all adding. I fully agree that completionism should not be a goal. Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [FFmpeg-cvslog] avfilter/af_ladspa: check another directory for plugins
Hi Paul!
On 2020-06-21 12:49 +, Paul B Mahol wrote:
> ffmpeg | branch: master | Paul B Mahol | Sun Jun 21
> 14:46:29 2020 +0200| [842bc312ade8fab82465423b22c4fbe3bee63383] | committer:
> Paul B Mahol
>
> avfilter/af_ladspa: check another directory for plugins
>
> > http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=842bc312ade8fab82465423b22c4fbe3bee63383
> ---
>
> libavfilter/af_ladspa.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/libavfilter/af_ladspa.c b/libavfilter/af_ladspa.c
> index a8562fc073..68537c5029 100644
> --- a/libavfilter/af_ladspa.c
> +++ b/libavfilter/af_ladspa.c
> @@ -426,6 +426,11 @@ static av_cold int init(AVFilterContext *ctx)
> }
>
> av_free(paths);
> +if (!s->dl_handle && (paths = av_asprintf("%s/.ladspa",
> getenv("HOME" {
I think this will result in undefined behavior by using %s with a NULL
argument if HOME isn't found in the environment.
As there's at least one more occurrence (see diff context below), it
would probably be best to define a file local helper function.
Alexander
> +s->dl_handle = try_load(paths, s->dl_name);
> +av_free(paths);
> +}
> +
> if (!s->dl_handle && (paths = av_asprintf("%s/.ladspa/lib",
> getenv("HOME" {
> s->dl_handle = try_load(paths, s->dl_name);
> av_free(paths);
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [FFmpeg-cvslog] avfilter/af_ladspa: check another directory for plugins
On 2020-06-21 21:19 +0200, Alexander Strasser wrote:
> On 2020-06-21 12:49 +, Paul B Mahol wrote:
> > ffmpeg | branch: master | Paul B Mahol | Sun Jun 21
> > 14:46:29 2020 +0200| [842bc312ade8fab82465423b22c4fbe3bee63383] |
> > committer: Paul B Mahol
> >
> > avfilter/af_ladspa: check another directory for plugins
> >
> > > http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=842bc312ade8fab82465423b22c4fbe3bee63383
> > ---
> >
> > libavfilter/af_ladspa.c | 5 +
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/libavfilter/af_ladspa.c b/libavfilter/af_ladspa.c
> > index a8562fc073..68537c5029 100644
> > --- a/libavfilter/af_ladspa.c
> > +++ b/libavfilter/af_ladspa.c
> > @@ -426,6 +426,11 @@ static av_cold int init(AVFilterContext *ctx)
> > }
> >
> > av_free(paths);
> > +if (!s->dl_handle && (paths = av_asprintf("%s/.ladspa",
> > getenv("HOME" {
>
> I think this will result in undefined behavior by using %s with a NULL
> argument if HOME isn't found in the environment.
>
> As there's at least one more occurrence (see diff context below), it
> would probably be best to define a file local helper function.
Ah, so the occurrence below was the only other one.
Thanks for fixing this quickly in commit fdac3c80ac65f !
Alexander
>
> > +s->dl_handle = try_load(paths, s->dl_name);
> > +av_free(paths);
> > +}
> > +
> > if (!s->dl_handle && (paths = av_asprintf("%s/.ladspa/lib",
> > getenv("HOME" {
> > s->dl_handle = try_load(paths, s->dl_name);
> > av_free(paths);
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [FFmpeg-cvslog] lavc/aac_ac3_parser: improve the raw AAC file bit rate calculation
On 2020-06-26 01:56 +, Jun Zhao wrote:
> ffmpeg | branch: master | Jun Zhao | Sun May 17
> 12:10:05 2020 +0800| [60d79b1df9d4c6030010ccb0c134ede9e33158c2] | committer:
> Jun Zhao
>
> lavc/aac_ac3_parser: improve the raw AAC file bit rate calculation
>
> Now we just use one ADTS raw frame to calculate the bit rate, it's
> lead to a larger error when get the duration from bit rate, the
> improvement cumulate Nth ADTS frames to get the average bit rate.
>
> e,g used the command get the duration like:
> ffprobe -show_entries format=duration -i fate-suite/aac/foo.aac
>
> before this improvement dump the duration=2.173935
> after this improvement dump the duration=1.979267
>
> in fact, the real duration can be get by command like:
> ffmpeg -i fate-suite/aac/foo.aac -f null /dev/null with time=00:00:01.97
>
> Also update the fate-adtstoasc_ticket3715.
>
> Signed-off-by: Jun Zhao
>
> > http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=60d79b1df9d4c6030010ccb0c134ede9e33158c2
> ---
>
> libavcodec/aac_ac3_parser.c | 9 +++--
> libavcodec/aac_ac3_parser.h | 2 ++
> tests/ref/fate/adtstoasc_ticket3715 | 2 +-
> 3 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/libavcodec/aac_ac3_parser.c b/libavcodec/aac_ac3_parser.c
> index 54e459844f..0746798dab 100644
> --- a/libavcodec/aac_ac3_parser.c
> +++ b/libavcodec/aac_ac3_parser.c
> @@ -97,8 +97,13 @@ get_next:
> avctx->audio_service_type = s->service_type;
> }
>
> -if (avctx->codec_id != AV_CODEC_ID_EAC3)
> -avctx->bit_rate = s->bit_rate;
> +/* Calculate the average bit rate */
> +s->frame_number++;
> +if (avctx->codec_id != AV_CODEC_ID_EAC3) {
> +avctx->bit_rate =
> +(s->last_bit_rate * (s->frame_number -1) +
> s->bit_rate)/s->frame_number;
> +s->last_bit_rate = avctx->bit_rate;
> +}
> }
Wouldn't it be better to sum up the individual bit_rate values in
a private context variable and divide by number of frames?
This way or the other, it might be useful to think about maximum
values of the total bits? I suspect the current int calculation
might overflow.
Alexander
> return i;
> diff --git a/libavcodec/aac_ac3_parser.h b/libavcodec/aac_ac3_parser.h
> index c2506a5bfd..b04041f69d 100644
> --- a/libavcodec/aac_ac3_parser.h
> +++ b/libavcodec/aac_ac3_parser.h
> @@ -55,6 +55,8 @@ typedef struct AACAC3ParseContext {
> uint64_t state;
>
> int need_next_header;
> +int frame_number;
> +int last_bit_rate;
> enum AVCodecID codec_id;
> } AACAC3ParseContext;
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [FFmpeg-cvslog] lavc/aac_ac3_parser: improve the raw AAC file bit rate calculation
On 2020-06-28 21:10 +0800, myp...@gmail.com wrote:
> On Sun, Jun 28, 2020 at 5:30 AM Alexander Strasser wrote:
> >
> > On 2020-06-26 01:56 +, Jun Zhao wrote:
> > > ffmpeg | branch: master | Jun Zhao | Sun May 17
> > > 12:10:05 2020 +0800| [60d79b1df9d4c6030010ccb0c134ede9e33158c2] |
> > > committer: Jun Zhao
> > >
[...]
> > >
> > > diff --git a/libavcodec/aac_ac3_parser.c b/libavcodec/aac_ac3_parser.c
> > > index 54e459844f..0746798dab 100644
> > > --- a/libavcodec/aac_ac3_parser.c
> > > +++ b/libavcodec/aac_ac3_parser.c
> > > @@ -97,8 +97,13 @@ get_next:
> > > avctx->audio_service_type = s->service_type;
> > > }
> > >
> > > -if (avctx->codec_id != AV_CODEC_ID_EAC3)
> > > -avctx->bit_rate = s->bit_rate;
> > > +/* Calculate the average bit rate */
> > > +s->frame_number++;
> > > +if (avctx->codec_id != AV_CODEC_ID_EAC3) {
> > > +avctx->bit_rate =
> > > +(s->last_bit_rate * (s->frame_number -1) +
> > > s->bit_rate)/s->frame_number;
> > > +s->last_bit_rate = avctx->bit_rate;
> > > +}
> > > }
> >
> > Wouldn't it be better to sum up the individual bit_rate values in
> > a private context variable and divide by number of frames?
> >
> I can't found a way in private context, so I change the AAC/AC3 parser
My wording probably wasn't perfect, I just meant in AACAC3ParseContext.
> > This way or the other, it might be useful to think about maximum
> > values of the total bits? I suspect the current int calculation
> > might overflow.
> >
> Will fix the potential overflow for int calculation
Please pardon me in advance. This is a bit of a rushed comment, but
as you are already working on this, I prefer to comment now.
One way to avoid the overflow and the multiplication could look like
this (not tested, may contain typos/errors etc.):
avctx->bit_rate += (s->bit_rate - s->last_bit_rate) / s->frame_number;
s->last_bit_rate = avctx->bit_rate;
This should yield almost identical results to the algorithm you
implemented. I say almost because when the increment is negative
the rounding is different.
Anyway it would also share the same flaw, that it would overvalue
the first frames and later frames will have diminishing impact.
On a further note, I can't believe this is the only place we have
this kind of problem. I think it would be good to investigate, how
it is solved elsewhere in the code base.
Alexander
> > > return i;
> > > diff --git a/libavcodec/aac_ac3_parser.h b/libavcodec/aac_ac3_parser.h
> > > index c2506a5bfd..b04041f69d 100644
> > > --- a/libavcodec/aac_ac3_parser.h
> > > +++ b/libavcodec/aac_ac3_parser.h
> > > @@ -55,6 +55,8 @@ typedef struct AACAC3ParseContext {
> > > uint64_t state;
> > >
> > > int need_next_header;
> > > +int frame_number;
> > > +int last_bit_rate;
> > > enum AVCodecID codec_id;
> > > } AACAC3ParseContext;
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v3 1/2] libavcodec/pgxdec: Add PGX decoder
On 2020-06-28 22:44 +0200, Michael Niedermayer wrote:
> On Sun, Jun 28, 2020 at 08:13:28PM +0530, gautamr...@gmail.com wrote:
> > From: Gautam Ramakrishnan
> >
> > This patch adds a pgx decoder.
> > ---
> > Changelog | 1 +
> > doc/general.texi| 2 +
> > libavcodec/Makefile | 1 +
> > libavcodec/allcodecs.c | 1 +
> > libavcodec/codec_desc.c | 7 ++
> > libavcodec/codec_id.h | 1 +
> > libavcodec/pgxdec.c | 205
> > libavcodec/version.h| 2 +-
> > 8 files changed, 219 insertions(+), 1 deletion(-)
> > create mode 100644 libavcodec/pgxdec.c
> >
> > diff --git a/Changelog b/Changelog
> > index a60e7d2eb8..1bb9931c0d 100644
> > --- a/Changelog
> > +++ b/Changelog
> > @@ -4,6 +4,7 @@ releases are sorted from youngest to oldest.
> > version :
> > - AudioToolbox output device
> > - MacCaption demuxer
> > +- PGX decoder
> >
> >
> > version 4.3:
> > diff --git a/doc/general.texi b/doc/general.texi
> > index ea34b963b5..53d556c56c 100644
> > --- a/doc/general.texi
> > +++ b/doc/general.texi
> > @@ -751,6 +751,8 @@ following image formats are supported:
> > @tab Portable GrayMap image
> > @item PGMYUV @tab X @tab X
> > @tab PGM with U and V components in YUV 4:2:0
> > +@item PGX @tab @tab X
> > +@tab PGX file decoder
> > @item PIC @tab @tab X
> > @tab Pictor/PC Paint
> > @item PNG @tab X @tab X
> > diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> > index 5a6ea59715..12aa43fe51 100644
> > --- a/libavcodec/Makefile
> > +++ b/libavcodec/Makefile
> > @@ -536,6 +536,7 @@ OBJS-$(CONFIG_PGM_ENCODER) += pnmenc.o
> > OBJS-$(CONFIG_PGMYUV_DECODER) += pnmdec.o pnm.o
> > OBJS-$(CONFIG_PGMYUV_ENCODER) += pnmenc.o
> > OBJS-$(CONFIG_PGSSUB_DECODER) += pgssubdec.o
> > +OBJS-$(CONFIG_PGX_DECODER) += pgxdec.o
> > OBJS-$(CONFIG_PICTOR_DECODER) += pictordec.o cga_data.o
> > OBJS-$(CONFIG_PIXLET_DECODER) += pixlet.o
> > OBJS-$(CONFIG_PJS_DECODER) += textdec.o ass.o
> > diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
> > index fa0c08d42e..a5048290f7 100644
> > --- a/libavcodec/allcodecs.c
> > +++ b/libavcodec/allcodecs.c
> > @@ -238,6 +238,7 @@ extern AVCodec ff_pgm_encoder;
> > extern AVCodec ff_pgm_decoder;
> > extern AVCodec ff_pgmyuv_encoder;
> > extern AVCodec ff_pgmyuv_decoder;
> > +extern AVCodec ff_pgx_decoder;
> > extern AVCodec ff_pictor_decoder;
> > extern AVCodec ff_pixlet_decoder;
> > extern AVCodec ff_png_encoder;
> > diff --git a/libavcodec/codec_desc.c b/libavcodec/codec_desc.c
> > index 9f8847544f..67e0a3055c 100644
> > --- a/libavcodec/codec_desc.c
> > +++ b/libavcodec/codec_desc.c
> > @@ -1405,6 +1405,13 @@ static const AVCodecDescriptor codec_descriptors[] =
> > {
> > .long_name = NULL_IF_CONFIG_SMALL("AVS2-P2/IEEE1857.4"),
> > .props = AV_CODEC_PROP_LOSSY,
> > },
> > +{
> > +.id= AV_CODEC_ID_PGX,
> > +.type = AVMEDIA_TYPE_VIDEO,
> > +.name = "pgx",
> > +.long_name = NULL_IF_CONFIG_SMALL("PGX (JPEG2000 Test Format)"),
> > +.props = AV_CODEC_PROP_INTRA_ONLY | AV_CODEC_PROP_LOSSLESS,
> > +},
> > {
> > .id= AV_CODEC_ID_Y41P,
> > .type = AVMEDIA_TYPE_VIDEO,
> > diff --git a/libavcodec/codec_id.h b/libavcodec/codec_id.h
> > index d885962c9c..896ecb0ce0 100644
> > --- a/libavcodec/codec_id.h
> > +++ b/libavcodec/codec_id.h
> > @@ -241,6 +241,7 @@ enum AVCodecID {
> > AV_CODEC_ID_SCREENPRESSO,
> > AV_CODEC_ID_RSCC,
> > AV_CODEC_ID_AVS2,
> > +AV_CODEC_ID_PGX,
> >
> > AV_CODEC_ID_Y41P = 0x8000,
> > AV_CODEC_ID_AVRP,
> > diff --git a/libavcodec/pgxdec.c b/libavcodec/pgxdec.c
> > new file mode 100644
> > index 00..688846f797
> > --- /dev/null
> > +++ b/libavcodec/pgxdec.c
> > @@ -0,0 +1,205 @@
> > +/*
> > + * PGX image format
> > + * Copyright (c) 2020 Gautam Ramakrishnan
> > + *
> > + * This file is part of FFmpeg.
> > + *
> > + * FFmpeg is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * FFmpeg is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with FFmpeg; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> > 02110-1301 USA
> > + */
> > +
> > +#include "avcodec.h"
> > +#include "internal.h"
> > +#include "bytestream.h"
>
Re: [FFmpeg-devel] [PATCH] ffprobe: Add option to allow unknown format private AVOptions
On 2020-06-28 22:23 +0200, Marton Balint wrote: > > > On Sun, 28 Jun 2020, Michael Niedermayer wrote: > > > On Sun, Jun 28, 2020 at 01:22:58PM +0100, Derek Buitenhuis wrote: > > > On 26/06/2020 14:49, Nicolas George wrote: > > > > Probably a good idea, but these explanation should probably go in > > > > doc/ffprobe.texi. > > > > > > Good point. Will add that during when I send v2. > > > > > > > And I do not maintain ffprobe. > > > > > > I have not seen Stefano active in a long time. Do you suggest I CC him on > > > the v2 patch? > > > > > > I always thought CCing people on patches was a bit rude, so I haven't. > > > > iam not who that question was directed to but i think ffprobe needs a > > new maintainer as Stefano seems (as you mention) not active ATM. > > looking at ffprobe git history, it seemed both myself and marton had > > applied patches from people other than themselfs to ffprobe "recently" so > > they did some ffprobe maintaince work. > > if marton wants to maintain ffprobe, i think noone would mind otherwise > > if not and noone else wants to fill that spot, i guess ffprobe is > > "community maintained" ATM. > > > > marton ? > > I try to keep track of most ffprobe changes, so fine by me if people agree. No disagreement here. If you are going forward with this, I think communication with Stefano would be a good idea. Not sure what others think, but if someone is listed as a maintainer I think contacting him about those areas is fine. Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavf/flvdec: normalize exporting date metadata
On 2021-05-10 10:22 +0200, Anton Khirnov wrote: > Export them in UTC, not the local timezone. This way the output is > the same everywhere. The timezone information stored in the file is > still ignored, since there seems to be no simple way to export it > correctly. > > Format them according to ISO 8601, which we generally use for exporting > dates. > --- > If anyone has practical suggestions for exporting the timezone, I would > love to hear them. Don't see anything in the standard library for > "express this UTC timestamp in a given timezone". Just adding the offset > to the timestamp would AFAIU not be correct wrt leap seconds (and maybe > something else? dates are hard). Surprisingly it seems best to ignore the timezone part on purpose! In 2.13 of Adobe's AMF 0 spec https://www.adobe.com/content/dam/acom/en/devnet/pdf/amf0-file-format-specification.pdf it is stated that the timezone part should not be filled, nor used. So the commit message part about it and the comment in the added by this patch should probably be removed. Though independent of this patch, one could maybe add a check that states sample welcome if a file with a timezone offset is found. > Also, the conversion of double to time_t is potentially UB if the source > value is out of range, but there seems to be no standard way to check, > since the range of time_t is not standard either. Anyone got any ideas? Not really. One could use sizeof to obtain the size, but for that to be useful one would need to assume signedness. The C standard doesn't seem to be explicit about that. And if it did, we still don't know if the functions handle negative offsets correctly. Seems that can't really by handled well locally here only Maybe we do already or could add a check in configure to find out about the bounds of time_t . Could be useful elsewhere in the codebase too. Maybe other have better ideas... Otherwise your patch looks good to me and fine without additional bound checks as it's not a new problem here. It could cause problems for people handling the timestamp in the currently exported format though. Maybe at least a micro bump should be in the final commit? Greetings, Alexander > --- > libavformat/flvdec.c | 7 +-- > tests/ref/fate/flv-demux | 2 +- > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c > index e6c2877a74..ddaceaafe4 100644 > --- a/libavformat/flvdec.c > +++ b/libavformat/flvdec.c > @@ -686,8 +686,11 @@ static int amf_parse_object(AVFormatContext *s, AVStream > *astream, > struct tm t; > char datestr[128]; > time = date.milliseconds / 1000; // to seconds > -localtime_r(&time, &t); > -strftime(datestr, sizeof(datestr), "%a, %d %b %Y %H:%M:%S %z", > &t); > +gmtime_r(&time, &t); > + > +// timezone is ignored, since there is no easy way to offset the > UTC > +// timestamp into the specified timezone > +strftime(datestr, sizeof(datestr), "%Y-%m-%dT%H:%M:%SZ", &t); > > av_dict_set(&s->metadata, key, datestr, 0); > } > diff --git a/tests/ref/fate/flv-demux b/tests/ref/fate/flv-demux > index 30435adeb9..827b56ea09 100644 > --- a/tests/ref/fate/flv-demux > +++ b/tests/ref/fate/flv-demux > @@ -605,4 +605,4 @@ > packet|codec_type=audio|stream_index=1|pts=11656|pts_time=11.656000|dts=11656|dt > > packet|codec_type=video|stream_index=0|pts=11678|pts_time=11.678000|dts=11678|dts_time=11.678000|duration=33|duration_time=0.033000|size=1190|pos=510794|flags=__|data_hash=CRC32:a0206c90 > > stream|index=0|codec_name=h264|profile=77|codec_type=video|codec_tag_string=[0][0][0][0]|codec_tag=0x|width=426|height=240|coded_width=426|coded_height=240|closed_captions=0|has_b_frames=1|sample_aspect_ratio=1:1|display_aspect_ratio=71:40|pix_fmt=yuv420p|level=21|color_range=unknown|color_space=unknown|color_transfer=unknown|color_primaries=unknown|chroma_location=left|field_order=progressive|refs=1|is_avc=true|nal_length_size=4|id=N/A|r_frame_rate=3/1001|avg_frame_rate=30/1|time_base=1/1000|start_pts=0|start_time=0.00|duration_ts=N/A|duration=N/A|bit_rate=393929|max_bit_rate=N/A|bits_per_raw_sample=8|nb_frames=N/A|nb_read_frames=N/A|nb_read_packets=351|extradata_hash=CRC32:07b85ca9|disposition:default=0|disposition:dub=0|disposition:original=0|disposition:comment=0|disposition:lyrics=0|disposition:karaoke=0|disposition:forced=0|disposition:hearing_impaired=0|disposition:visual_impaired=0|disposition:clean_effects=0|disposition:attached_pic=0|disposition:timed_thumbna il > > s=0|disposition:captions=0|disposition:descriptions=0|disposition:metadata=0|disposition:dependent=0|disposition:still_image=0 > > stream|index=1|codec_name=aac|profile=1|codec_type=audio|codec_tag_string=[0][0][0][0]|codec_tag=0x|sample_fmt=fltp|sample_rate=22050|channels=2|channel_layout=stereo|bits_per_sample=0|id=N/A|r_frame_rate=0/0
Re: [FFmpeg-devel] [PATCH] lavf/flvdec: normalize exporting date metadata
On 2021-05-11 17:51 +0200, Anton Khirnov wrote:
> Quoting Alexander Strasser (2021-05-10 15:35:02)
> > On 2021-05-10 10:22 +0200, Anton Khirnov wrote:
> > > Export them in UTC, not the local timezone. This way the output is
> > > the same everywhere. The timezone information stored in the file is
> > > still ignored, since there seems to be no simple way to export it
> > > correctly.
> > >
> > > Format them according to ISO 8601, which we generally use for exporting
> > > dates.
> > > ---
> > > If anyone has practical suggestions for exporting the timezone, I would
> > > love to hear them. Don't see anything in the standard library for
> > > "express this UTC timestamp in a given timezone". Just adding the offset
> > > to the timestamp would AFAIU not be correct wrt leap seconds (and maybe
> > > something else? dates are hard).
> >
> > Surprisingly it seems best to ignore the timezone part on purpose!
> >
> > In 2.13 of Adobe's AMF 0 spec
> >
> > https://www.adobe.com/content/dam/acom/en/devnet/pdf/amf0-file-format-specification.pdf
> >
> > it is stated that the timezone part should not be filled, nor
> > used.
> >
> > So the commit message part about it and the comment in the
> > added by this patch should probably be removed.
> >
> > Though independent of this patch, one could maybe add a
> > check that states sample welcome if a file with a timezone
> > offset is found.
>
> I have no idea how FLV is related to AMF0,
FLV reuses AMF0 spec for serialization of the metadata we talk about?
> but the sample used in the
> test in question here does have a non-zero value of the tz offset. So
> the comment seems appropriate to me.
Interesting.
In case of that sample it is probably FLVTool2 that wrote that
wrote the metadata. Then this should be its Ruby code for handling
the AMF date type:
def write__AMF_date(time)
write__UI8 11
write [(time.to_f * 1000.0)].pack('G')
write__SI16( (Time.now.gmtoff / 60).to_i )
end
def read__AMF_date
utc_time = Time.at((read__AMF_double / 1000).to_i)
utc_time + (read__SI16 * 60) - Time.now.gmtoff
end
I have a attached patch, that on top of your patch, adds output of
timezone information according to the quoted interpretation of
FLVTool2.
Best regards,
Alexander
[...]
From 3fd6c8f81baacace49e0a6cc431295dc56a077bc Mon Sep 17 00:00:00 2001
From: Alexander Strasser
Date: Wed, 12 May 2021 00:46:54 +0200
Subject: [PATCH] lavf/flvdec: metadata date: respect timezone information if
present
If the timezone data of an AMF 0 date type is non-zero, include that
information as UTC offset in hours and minutes.
---
libavformat/flvdec.c | 18 +++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c
index ddaceaafe4..c941e62e23 100644
--- a/libavformat/flvdec.c
+++ b/libavformat/flvdec.c
@@ -688,9 +688,21 @@ static int amf_parse_object(AVFormatContext *s, AVStream *astream,
time = date.milliseconds / 1000; // to seconds
gmtime_r(&time, &t);
-// timezone is ignored, since there is no easy way to offset the UTC
-// timestamp into the specified timezone
-strftime(datestr, sizeof(datestr), "%Y-%m-%dT%H:%M:%SZ", &t);
+strftime(datestr, sizeof(datestr), "%Y-%m-%dT%H:%M:%S", &t);
+
+if (date.timezone) {
+int off_tz = date.timezone; // offset in minutes
+char ch_sign = '+';
+if (off_tz < 0) {
+off_tz = -off_tz;
+ch_sign = '-';
+}
+if (off_tz > 99*60 + 59)
+off_tz = 99*60 + 59;
+
+av_strlcatf(datestr, sizeof(datestr), "%c%02d%02d", ch_sign, off_tz / 60, off_tz % 60);
+} else
+av_strlcat(datestr, "Z", sizeof(datestr));
av_dict_set(&s->metadata, key, datestr, 0);
}
--
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavf/flvdec: normalize exporting date metadata
Hi Anton!
On 2021-05-14 10:09 +0200, Anton Khirnov wrote:
> Quoting Alexander Strasser (2021-05-12 01:04:28)
> >
> > If the timezone data of an AMF 0 date type is non-zero, include that
> > information as UTC offset in hours and minutes.
> > ---
> > libavformat/flvdec.c | 18 +++---
> > 1 file changed, 15 insertions(+), 3 deletions(-)
> >
> > diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c
> > index ddaceaafe4..c941e62e23 100644
> > --- a/libavformat/flvdec.c
> > +++ b/libavformat/flvdec.c
> > @@ -688,9 +688,21 @@ static int amf_parse_object(AVFormatContext *s,
> > AVStream *astream,
> > time = date.milliseconds / 1000; // to seconds
> > gmtime_r(&time, &t);
> >
> > -// timezone is ignored, since there is no easy way to offset
> > the UTC
> > -// timestamp into the specified timezone
> > -strftime(datestr, sizeof(datestr), "%Y-%m-%dT%H:%M:%SZ", &t);
> > +strftime(datestr, sizeof(datestr), "%Y-%m-%dT%H:%M:%S", &t);
> > +
> > +if (date.timezone) {
> > +int off_tz = date.timezone; // offset in minutes
> > +char ch_sign = '+';
> > +if (off_tz < 0) {
> > +off_tz = -off_tz;
> > +ch_sign = '-';
> > +}
> > +if (off_tz > 99*60 + 59)
> > +off_tz = 99*60 + 59;
> > +
> > +av_strlcatf(datestr, sizeof(datestr), "%c%02d%02d",
> > ch_sign, off_tz / 60, off_tz % 60);
>
> I still believe this is wrong, since the spec says the timestamp is in
> UTC. The code you quoted seems to conform to that.
Not to my understanding and testing.
This Ruby program
t1 = Time.now()
t2 = Time.now.utc()
print t1, " - ", t1.to_f, "\n"
print t2, " - ", t2.to_f, "\n"
yields for example:
2021-05-15 20:05:19 +0200 - 1621101919.509961
2021-05-15 18:05:19 UTC - 1621101919.509966
Returning to the code I quoted before now and stating my
understanding of if now.
def write__AMF_date(time)
write__UI8 11
write [(time.to_f * 1000.0)].pack('G')
write__SI16( (Time.now.gmtoff / 60).to_i )
end
This writes the time in micro seconds without offset as double.
The GMT offset in minutes is written afterwards as signed 16 bit
integer.
def read__AMF_date
utc_time = Time.at((read__AMF_double / 1000).to_i)
utc_time + (read__SI16 * 60) - Time.now.gmtoff
end
This first reads the double and converts it into a Time object.
In the following line it reads and adds the stored offset and
subtracts the current offset to get rid of its influence.
Alexander
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavf/flvdec: normalize exporting date metadata
On 2021-05-16 21:18 +0200, Anton Khirnov wrote:
> Quoting Alexander Strasser (2021-05-15 20:20:30)
[...]
> >
> > Returning to the code I quoted before now and stating my
> > understanding of if now.
> >
> > def write__AMF_date(time)
> > write__UI8 11
> > write [(time.to_f * 1000.0)].pack('G')
> > write__SI16( (Time.now.gmtoff / 60).to_i )
> > end
> >
> > This writes the time in micro seconds without offset as double.
> > The GMT offset in minutes is written afterwards as signed 16 bit
> > integer.
> >
> >
> > def read__AMF_date
> > utc_time = Time.at((read__AMF_double / 1000).to_i)
> > utc_time + (read__SI16 * 60) - Time.now.gmtoff
> > end
> >
> > This first reads the double and converts it into a Time object.
> > In the following line it reads and adds the stored offset and
> > subtracts the current offset to get rid of its influence.
>
> The writing code looks correct, but the reading code seems suspicious.
Actually I think both are wrong. I was fooled before because the
resulting output of flvtool2 is wrong if the timezone data is zero.
> To test it, I used flvtool2 to write metadata with TZ=America/New_York
> (-04:00) at 19:08 UTC. Then reading it with TZ=Europe/Paris (+02:00)
> gives me:
> - ffmpeg 4.3.2 -- correct, prints date in local timezone
> metadatadate: Sun, 16 May 2021 21:08:41 +0200
> - current ffmpeg git master -- correct, prints date in UTC
> metadatadate: 2021-05-16T19:08:41Z
> - flvtool2 -- WRONG
> metadatadate: Sun May 16 15:08:41 GMT+0200 2021
>
> From which I conclude that flvtool2's own reading code is incorrect.
I'm not convinced that interpreting the timezone data will be a good
thing. Therefore I think the new comment in the code is probably not
good advice. I don't mind it much though, as at least for the files
written by flvtool2 it isn't wrong. I don't have any more examples
of this, despite tools writing a UTC timestamp and setting timezone
to zero always.
That the timezone offset written next to a UTC timestamp should
indicate the timezone of the system that wrote it seems unusual.
What useful information would that convey? Probably some vague
location information.
On the bright side: Current and previous FFmpeg code always did the
right thing for metadata dates written by flvtool2 :)
I withdraw my patch as from what we know now it would give wrong
output for metadata files that were generated by flvtool2.
FWIW I have opened a bug report for flvtool2:
https://github.com/unnu/flvtool2/issues/10
Though it seems not to be maintained since around 2014.
Thanks for looking into this, Anton.
Best regards,
Alexander
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH] libavdevice/v4l2: Return EAGAIN when receiving a frame of unexpected size
I had found that when capturing video for some hours from USB Camera-B4.09.24.1 (Manufacturer: OmniVision Technologies, Inc.), sometimes when invoking the ioctl DQBUF, the returned buffer is not filled with the size we expect for the raw video frame. Here are two examples for such occurrences: [video4linux2,v4l2 @ 0x258b440] Dequeued v4l2 buffer contains 609596 bytes, but 614400 were expected. Flags: 0x0001. /dev/video1: Invalid data found when processing input [video4linux2,v4l2 @ 0x225f440] Dequeued v4l2 buffer contains 609508 bytes, but 614400 were expected. Flags: 0x0001. /dev/video1: Invalid data found when processing input The ffmpeg CLI tool will then stop capturing and exit. To avoid this, return AVERROR(EAGAIN) instead. While searching for the above quoted error message I found a ticket that mentions this type of problem on our tracker. Probably fixes #4795 --- We re-discovered this problem that was reported many years ago, while running the motion vector demo at Chemnitzer Linux-Tage yesterday and today. libavdevice/v4l2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c index 1b9c6e7..894692e 100644 --- a/libavdevice/v4l2.c +++ b/libavdevice/v4l2.c @@ -542,7 +542,7 @@ static int mmap_read_frame(AVFormatContext *ctx, AVPacket *pkt) "Dequeued v4l2 buffer contains %d bytes, but %d were expected. Flags: 0x%08X.\n", buf.bytesused, s->frame_size, buf.flags); enqueue_buffer(s, &buf); -return AVERROR_INVALIDDATA; +return AVERROR(EAGAIN); } } -- ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] libavdevice/v4l2: Return EAGAIN when receiving a frame of unexpected size
On 2019-03-17 18:25 +0100, Nicolas George wrote: > Alexander Strasser (12019-03-17): > > I had found that when capturing video for some hours from > > USB Camera-B4.09.24.1 (Manufacturer: OmniVision Technologies, Inc.), > > sometimes when invoking the ioctl DQBUF, the returned buffer is not > > filled with the size we expect for the raw video frame. > > Which seems like a kernel bug, does it not? Seems like a kernel bug, yes. Probably driver specific. Though I am not 100% sure it is strictly a bug. Might be that this is the best a driver for a particular hardware can do. In any case at least 2 different and probably more devices exhibited the described behaviour, which was never a permanent failure. I suspect some drivers will always exhibit this behavior sometimes. That's why I think we should handle this particular case more graceful. > > To avoid this, return AVERROR(EAGAIN) instead. > > Returning EAGAIN when not in non-blocking mode is invalid. FFERROR_REDO > would be the correct code in that case. Maybe using EAGAIN wasn't a good choice :( Using FFERROR_REDO sounds promising. I will try that. I tested the EAGAIN version and it worked. I also tested returning a zero-sized packet, like in the case where V4L flags the data as corrupt, that worked too. See commit 28f20d2ff487aa589643d8f70eaf614b78839685 Do you think the zero-sized packet would be better than returning FFERROR_REDO? Thanks for your comments! Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] libavdevice/v4l2: Return EAGAIN when receiving a frame of unexpected size
Hi Nicolas! On 2019-03-18 00:24 +0100, Nicolas George wrote: > Alexander Strasser (12019-03-18): > > I tested the EAGAIN version and it worked. I also tested returning a > > ffmpeg.c uses the non-blocking mode. That would explain it. I now tested, the FFERROR_REDO approach, and I think it works fine. As I don't have it available anymore, I can't test with the device that gave me the errors. So I modified the code to create the error condition. Using FFERROR_REDO would work for blocking mode as well as non-blocking, right? It handles the error on a lower level inside the libs and doesn't bubble up to the lib user AFAICT. > > zero-sized packet, like in the case where V4L flags the data as corrupt, > > that worked too. See commit 28f20d2ff487aa589643d8f70eaf614b78839685 > > > > Do you think the zero-sized packet would be better than returning > > FFERROR_REDO? > > I think it depends on what happens exactly with that frame? What is the > partial packet returned? Is there a timestamp? Etc. As mentioned above I can't dump more data to get a clue. I guess the frame was just truncated and the timestamps were correct. As we wouldn't pass on frame data to the user anyway, I would actually prefer the FFERROR_REDO version. Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] lavd/v4l2: Return FFERROR_REDO when receiving a frame of unexpected size
I had found that when capturing video for some hours from USB Camera-B4.09.24.1 (Manufacturer: OmniVision Technologies, Inc.), sometimes when invoking the ioctl DQBUF, the returned buffer is not filled with the size we expect for the raw video frame. Here are two examples for such occurrences: [video4linux2,v4l2 @ 0x258b440] Dequeued v4l2 buffer contains 609596 bytes, but 614400 were expected. Flags: 0x0001. /dev/video1: Invalid data found when processing input [video4linux2,v4l2 @ 0x225f440] Dequeued v4l2 buffer contains 609508 bytes, but 614400 were expected. Flags: 0x0001. /dev/video1: Invalid data found when processing input The ffmpeg CLI tool will then stop capturing and exit. To avoid this, return FFERROR_REDO instead. I have not seen the issue appearing twice or more often in a row. It was more like one single error every two hours. While searching for the above quoted error message I found a ticket that mentions this type of problem on our tracker. Fixes #4795 --- libavdevice/v4l2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c index 1b9c6e7..5a1b324 100644 --- a/libavdevice/v4l2.c +++ b/libavdevice/v4l2.c @@ -542,7 +542,7 @@ static int mmap_read_frame(AVFormatContext *ctx, AVPacket *pkt) "Dequeued v4l2 buffer contains %d bytes, but %d were expected. Flags: 0x%08X.\n", buf.bytesused, s->frame_size, buf.flags); enqueue_buffer(s, &buf); -return AVERROR_INVALIDDATA; +return FFERROR_REDO; } } -- ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] libavdevice/v4l2: Return EAGAIN when receiving a frame of unexpected size
On 2019-03-18 23:26 +0100, Alexander Strasser wrote: > On 2019-03-18 00:24 +0100, Nicolas George wrote: > > Alexander Strasser (12019-03-18): > > > I tested the EAGAIN version and it worked. I also tested returning a > > > > ffmpeg.c uses the non-blocking mode. > > That would explain it. I now tested, the FFERROR_REDO approach, > and I think it works fine. As I don't have it available anymore, > I can't test with the device that gave me the errors. So I modified > the code to create the error condition. > > Using FFERROR_REDO would work for blocking mode as well as non-blocking, > right? > > It handles the error on a lower level inside the libs and doesn't > bubble up to the lib user AFAICT. > > > > > zero-sized packet, like in the case where V4L flags the data as corrupt, > > > that worked too. See commit 28f20d2ff487aa589643d8f70eaf614b78839685 > > > > > > Do you think the zero-sized packet would be better than returning > > > FFERROR_REDO? > > > > I think it depends on what happens exactly with that frame? What is the > > partial packet returned? Is there a timestamp? Etc. > > As mentioned above I can't dump more data to get a clue. I guess > the frame was just truncated and the timestamps were correct. > > As we wouldn't pass on frame data to the user anyway, I would actually > prefer the FFERROR_REDO version. I sent a new patch changing the returned error code to FFERROR_REDO. This patch can be considered rejected. Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] lavd/v4l2: Return FFERROR_REDO when receiving a frame of unexpected size
Hi all! On 2019-03-20 19:56 +0100, Alexander Strasser wrote: > I had found that when capturing video for some hours from > USB Camera-B4.09.24.1 (Manufacturer: OmniVision Technologies, Inc.), > sometimes when invoking the ioctl DQBUF, the returned buffer is not > filled with the size we expect for the raw video frame. > > Here are two examples for such occurrences: > > [video4linux2,v4l2 @ 0x258b440] Dequeued v4l2 buffer contains 609596 > bytes, but 614400 were expected. Flags: 0x0001. > /dev/video1: Invalid data found when processing input > > [video4linux2,v4l2 @ 0x225f440] Dequeued v4l2 buffer contains 609508 > bytes, but 614400 were expected. Flags: 0x0001. > /dev/video1: Invalid data found when processing input > > The ffmpeg CLI tool will then stop capturing and exit. > > To avoid this, return FFERROR_REDO instead. I have not seen the > issue appearing twice or more often in a row. It was more like > one single error every two hours. After thinking about the FFERROR_REDO approach some more, I think it is not ideal in the case where a driver permanently delivers frames of unexpected size. The FFmpeg lib call would not return to the client, potentially freezing the running program. So maybe the zero-sized package approach, like we do with the corrupted frames, is more robust here? Any opinions? Best regards, Alexander > > While searching for the above quoted error message I found a ticket > that mentions this type of problem on our tracker. > > Fixes #4795 > --- > libavdevice/v4l2.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c > index 1b9c6e7..5a1b324 100644 > --- a/libavdevice/v4l2.c > +++ b/libavdevice/v4l2.c > @@ -542,7 +542,7 @@ static int mmap_read_frame(AVFormatContext *ctx, AVPacket > *pkt) > "Dequeued v4l2 buffer contains %d bytes, but %d were > expected. Flags: 0x%08X.\n", > buf.bytesused, s->frame_size, buf.flags); > enqueue_buffer(s, &buf); > -return AVERROR_INVALIDDATA; > +return FFERROR_REDO; > } > } > > -- ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] lavd/v4l2: Return FFERROR_REDO when receiving a frame of unexpected size
Hi Marton, hi Nicolas! On 2019-03-21 10:07 +0100, Nicolas George wrote: > Marton Balint (12019-03-21): > > Maybe just set the packet corrupt flag and log an error, but return the > > partial buffer? > > I think it may be the best solution. But it would be interesting to know > how the tools react to it. I agree that it might be the most sophisticated solution. But it is not so clear how this would work out with ffmpeg and all other client apps. It might also be more tricky/involved to implement correctly. Personally I don't want to invest much more time into this ATM. I would really like to achieve to prevent aborting the capture in case of a frame once in a while being returned too short. I believe it would be a big win that can hopefully be implemented without causing harm. After considering the 3 variants I have implemented so far I am now at the following conclusion: 1. Return AVERROR(EAGAIN) => this is not an option because it is not expected when using blocking mode 2. Return FFERROR_REDO => this could be used, but has the not completely unlikely downside to not return to the caller for a very long time. That is in the case the driver starts emitting too short frames in a row for a long time. 3. Return zero-sized packets => This works and is consistent with how we handle frames flagged to be corrupted (V4L2_BUF_FLAG_ERROR). See commit 28f20d2ff487aa589643d8f70eaf614b78839685 . Choosing from the above 3 options, IMHO 3 is the best. If someone decides to improve it at a later point, one could improve the V4L2_BUF_FLAG_ERROR case too. Do you think that going with 3 now is fine? If yes I would send that patch for review. Thanks for your comments! Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] avformat/dashdec: add dash demuxer base version
On 2017-03-19 11:01 +0100, wm4 wrote: > On Sun, 19 Mar 2017 10:57:21 +0100 > Clément Bœsch wrote: > > > > > If you want to have DASH enabled by default, make sure to have a native > > XML parser, or discuss the policy about detecting all the external > > libraries within the configure. > > > > That's ridiculous. So NIH is encouraged because of a crappy policy > about using external libs? Please... No need to exaggerate... You even quoted the "or discuss policy" part of the sentence. Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avcodec/h264: Check weight values to be within the specs limits.
On 2017-03-22 09:06 -0400, Ronald S. Bultje wrote:
> On Tue, Mar 21, 2017 at 9:59 PM, Michael Niedermayer > wrote:
>
> > @@ -59,6 +59,9 @@ int ff_h264_pred_weight_table(GetBitContext *gb, const
> > SPS *sps,
> > if (luma_weight_flag) {
> > pwt->luma_weight[i][list][0] = get_se_golomb(gb);
> > pwt->luma_weight[i][list][1] = get_se_golomb(gb);
> > +if ( (int8_t)pwt->luma_weight[i][list][0] !=
> > pwt->luma_weight[i][list][0]
> > +|| (int8_t)pwt->luma_weight[i][list][1] !=
> > pwt->luma_weight[i][list][1])
> > +goto error;
> >
>
> Can we please put || on the line before? h264* and a very limited subset of
> other files in our codebase have always been an exception to the large
> majority of the codebase and it's about time we fix that [1].
No strong opinion on this one, but putting operators in front on the next
line makes them stay aligned in case a name changes later on. I think it is
a little bit easier to read too.
I did not check the counts, but I guess putting the logical operators at
the end is used far more common in our code base and in no way do I suggest
we should change all of those occurrences to this style.
[...]
Alexander
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 01/10] lavu: add av_fourcc_make_string() and av_4cc2str()
Hi Clément,
I think your idea of introducing this function and the convenience macro is
good.
Below are some comments, feel free to ignore.
On 2017-03-27 09:51 +0200, Clément Bœsch wrote:
> ---
> doc/APIchanges | 4
> libavutil/avutil.h | 14 ++
> libavutil/utils.c | 21 +
> libavutil/version.h | 2 +-
> 4 files changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 6aaa9adceb..4736e3e6fc 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,10 @@ libavutil: 2015-08-28
>
> API changes, most recent first:
>
> +2017-03-xx - xxx - lavu 55.52.100 - avutil.h
> + add av_fourcc_make_string() function and av_4cc2str() macro to replace
> + av_get_codec_tag_string() from lavc.
> +
> 2017-03-xx - xxx - lavc 57.85.101 - avcodec.h
>vdpau hardware accelerated decoding now supports the new hwaccel API, which
>can create the decoder context and allocate hardware frame automatically.
> diff --git a/libavutil/avutil.h b/libavutil/avutil.h
> index e9aaa03722..98100fdcc5 100644
> --- a/libavutil/avutil.h
> +++ b/libavutil/avutil.h
> @@ -343,6 +343,20 @@ FILE *av_fopen_utf8(const char *path, const char *mode);
> */
> AVRational av_get_time_base_q(void);
>
> +#define AV_FOURCC_MAX_STRING_SIZE 32
Should be fine, though I don't know how you came up with this max size in
particular.
> +
> +#define av_4cc2str(fourcc)
> av_fourcc_make_string((char[AV_FOURCC_MAX_STRING_SIZE]){0}, fourcc)
Did your write it as 4cc2str to make the name shorter?
I guess fourcc2str is more readable and more consistent.
> +
> +/**
> + * Fill the provided buffer with a string containing a FourCC (four-character
> + * code) representation.
> + *
> + * @param bufa buffer with size in bytes of at least
> AV_FOURCC_MAX_STRING_SIZE
> + * @param fourcc the fourcc to represent
> + * @return the buffer in input
> + */
> +char *av_fourcc_make_string(char *buf, uint32_t fourcc);
> +
> /**
> * @}
> * @}
> diff --git a/libavutil/utils.c b/libavutil/utils.c
> index 36e4dd5fdb..ba557b9252 100644
> --- a/libavutil/utils.c
> +++ b/libavutil/utils.c
> @@ -121,6 +121,27 @@ unsigned av_int_list_length_for_size(unsigned elsize,
> return i;
> }
>
> +char *av_fourcc_make_string(char *buf, uint32_t fourcc)
> +{
> +int i, len;
> +char *orig_buf = buf;
> +size_t buf_size = AV_FOURCC_MAX_STRING_SIZE;
> +
> +#define TAG_PRINT(x) \
> +(((x) >= '0' && (x) <= '9') ||\
> + ((x) >= 'a' && (x) <= 'z') || ((x) >= 'A' && (x) <= 'Z') || \
> + ((x) == '.' || (x) == ' ' || (x) == '-' || (x) == '_'))
You could spare this macro, by using a temporary char for the character and
an int to indicate if it is printable in the loop below. Would probably be
1 or 2 lines longer.
You might want to at least rename TAG_PRINT to IS_PRINTABLE or
IS_PRINTABLE_FOURCC_CHAR or similar as TAG_PRINT is a rather confusing name.
Also if the macro is only introduced for use this function, #undef could be
used, but I don't think we really do it anywhere else.
> +
> +for (i = 0; i < 4; i++) {
> +len = snprintf(buf, buf_size,
> + TAG_PRINT(fourcc & 0xFF) ? "%c" : "[%d]", fourcc &
> 0xFF);
> +buf += len;
> +buf_size = buf_size > len ? buf_size - len : 0;
> +fourcc >>= 8;
> +}
> +return orig_buf;
> +}
> +
> AVRational av_get_time_base_q(void)
> {
> return (AVRational){1, AV_TIME_BASE};
[...]
Alexander
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 01/10] lavu: add av_fourcc_make_string() and av_4cc2str()
On 2017-03-28 10:19 +0200, Clément Bœsch wrote:
> On Tue, Mar 28, 2017 at 12:47:39AM +0200, Alexander Strasser wrote:
> [...]
> > > +#define AV_FOURCC_MAX_STRING_SIZE 32
> >
> > Should be fine, though I don't know how you came up with this max size in
> > particular.
> >
>
> IIRC the actual maximum is 21 characters, I just took the closest superior
> power of two. Probably just like we did for AV_TS_MAX_STRING_SIZE.
OK
> > > +
> > > +#define av_4cc2str(fourcc)
> > > av_fourcc_make_string((char[AV_FOURCC_MAX_STRING_SIZE]){0}, fourcc)
> >
> > Did your write it as 4cc2str to make the name shorter?
> > I guess fourcc2str is more readable and more consistent.
> >
>
> This is a shorthand, so yeah I wanted to keep it short. If you replace
> "4cc" with "fourcc", why not do that for the '2' as well? Writing "four"
> but keeping '2' was kind of weird to me :)
Well, to expand on my point a bit. The 2 in these kind of functions works
like a delimiter to me e.g. "a2b"; easy to tokenize. When looking at "4a2b",
it is a bit harder. I know the example is a bit more extreme.
The other thing is that 4 is also sometimes used as meaning "for", similar
to the usage of 2 as meaning "to".
TL;DR: I am fine with the name you chose, just wanted to tell my impression.
Regarding the follow-up patches renaming would also cause a fair bit of labour
to you. So I can totally understand if you keep the name.
> > > +
> > > +/**
> > > + * Fill the provided buffer with a string containing a FourCC
> > > (four-character
> > > + * code) representation.
> > > + *
> > > + * @param bufa buffer with size in bytes of at least
> > > AV_FOURCC_MAX_STRING_SIZE
> > > + * @param fourcc the fourcc to represent
> > > + * @return the buffer in input
> > > + */
> > > +char *av_fourcc_make_string(char *buf, uint32_t fourcc);
> > > +
> > > /**
> > > * @}
> > > * @}
> > > diff --git a/libavutil/utils.c b/libavutil/utils.c
> > > index 36e4dd5fdb..ba557b9252 100644
> > > --- a/libavutil/utils.c
> > > +++ b/libavutil/utils.c
> > > @@ -121,6 +121,27 @@ unsigned av_int_list_length_for_size(unsigned elsize,
> > > return i;
> > > }
> > >
> > > +char *av_fourcc_make_string(char *buf, uint32_t fourcc)
> > > +{
> > > +int i, len;
> > > +char *orig_buf = buf;
> > > +size_t buf_size = AV_FOURCC_MAX_STRING_SIZE;
> > > +
> > > +#define TAG_PRINT(x) \
> > > +(((x) >= '0' && (x) <= '9') ||\
> > > + ((x) >= 'a' && (x) <= 'z') || ((x) >= 'A' && (x) <= 'Z') || \
> > > + ((x) == '.' || (x) == ' ' || (x) == '-' || (x) == '_'))
> >
> > You could spare this macro, by using a temporary char for the character and
> > an int to indicate if it is printable in the loop below. Would probably be
> > 1 or 2 lines longer.
> >
> > You might want to at least rename TAG_PRINT to IS_PRINTABLE or
> > IS_PRINTABLE_FOURCC_CHAR or similar as TAG_PRINT is a rather confusing name.
> >
> > Also if the macro is only introduced for use this function, #undef could be
> > used, but I don't think we really do it anywhere else.
> >
>
> Yeah well I just took the existing code in libavcodec, so I didn't want to
> change it too much.
>
> But you're right on all your comments so I just adjusted the function. See
> patch attached.
I will comment on the patch in response to Michael's comment.
Alexander
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 01/10] lavu: add av_fourcc_make_string() and av_4cc2str()
On 2017-03-28 18:16 +0200, Clément Bœsch wrote:
> On Tue, Mar 28, 2017 at 06:12:14PM +0200, Michael Niedermayer wrote:
> > On Tue, Mar 28, 2017 at 10:19:46AM +0200, Clément Bœsch wrote:
> > [...]
> > > diff --git a/libavutil/utils.c b/libavutil/utils.c
> > > index 36e4dd5fdb..29f2746338 100644
> > > --- a/libavutil/utils.c
> > > +++ b/libavutil/utils.c
> > > @@ -121,6 +121,29 @@ unsigned av_int_list_length_for_size(unsigned elsize,
> > > return i;
> > > }
> > >
> > > +char *av_fourcc_make_string(char *buf, uint32_t fourcc)
> > > +{
> > > +int i;
> > > +char *orig_buf = buf;
> > > +size_t buf_size = AV_FOURCC_MAX_STRING_SIZE;
> > > +
> > > +for (i = 0; i < 4; i++) {
> > > +const char c = fourcc & 0xff;
> > > +const int print_chr = (c >= '0' && c <= '9') ||
> > > + (c >= 'a' && c <= 'z') ||
> > > + (c >= 'A' && c <= 'Z') ||
> > > + (c && strchr(". -_", c));
> > > +const int len = snprintf(buf, buf_size, print_chr ? "%c" :
> > > "[%d]", c);
> >
> > this prints values over 127 as negative if char is signed
> >
>
> oh i thought i changed that to const int c = ...
> forgot to resend the patch, consider it fixed locally
Sorry, I probably hinted you in the wrong direction there. It came to my
mind while brushing my teeth yesterday. Using int for the var holding the
byte value should be sufficient and especially avoid the unwanted sign
extension.
LGTM with the fix you mentioned; thank you for making the adjustments!
Alexander
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] pthread_frame: minor simplification to error handling
Hi,
I already saw this on -cvslog ml, so apparently I am too late...
On 2017-03-29 12:19 +0200, wm4 wrote:
> On Mon, 27 Mar 2017 14:28:17 +0200
> wm4 wrote:
>
> > Get rid of the "ret" variable, and always use err. Report the packet as
> > consumed if err is unset. This should be equivalent to the old code,
> > which obviously required err=0 for p->result>=0 (and otherwise,
> > p->result must have had the value err was last set to). The code block
> > added by commit 32a5b631267 is also not needed anymore, because the new
> > code strictly returns err if it's >=0.
> > ---
> > Not totally sure about this, but it seems to work out? I probably missed
> > something obvious.
> > ---
> > libavcodec/pthread_frame.c | 19 +--
> > 1 file changed, 5 insertions(+), 14 deletions(-)
> >
> > diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
> > index b618be0bf5..36e4a8affa 100644
> > --- a/libavcodec/pthread_frame.c
> > +++ b/libavcodec/pthread_frame.c
> > @@ -468,7 +468,7 @@ int ff_thread_decode_frame(AVCodecContext *avctx,
> > FrameThreadContext *fctx = avctx->internal->thread_ctx;
> > int finished = fctx->next_finished;
> > PerThreadContext *p;
> > -int err, ret = 0;
> > +int err;
Wouldn't it have been more readable to keep ret instead of err?
Assigning a size to err looks a bit strange. I am not insisting on changing
it now, just wanted to point this out.
> >
> > /* release the async lock, permitting blocked hwaccel threads to
> > * go forward while we are in this function */
> > @@ -496,7 +496,7 @@ int ff_thread_decode_frame(AVCodecContext *avctx,
> > if (fctx->delaying) {
> > *got_picture_ptr=0;
> > if (avpkt->size) {
> > -ret = avpkt->size;
> > +err = avpkt->size;
> > goto finish;
> > }
> > }
> > @@ -542,21 +542,12 @@ int ff_thread_decode_frame(AVCodecContext *avctx,
> >
> > fctx->next_finished = finished;
> >
> > -/*
> > - * When no frame was found while flushing, but an error occurred in
> > - * any thread, return it instead of 0.
> > - * Otherwise the error can get lost.
> > - */
> > -if (!avpkt->size && !*got_picture_ptr)
> > -goto finish;
> > -
> > /* return the size of the consumed packet if no error occurred */
> > -ret = (p->result >= 0) ? avpkt->size : p->result;
> > +if (err >= 0)
> > +err = avpkt->size;
> > finish:
> > async_lock(fctx);
> > -if (err < 0)
> > -return err;
> > -return ret;
> > +return err;
> > }
> >
> > void ff_thread_report_progress(ThreadFrame *f, int n, int field)
>
> Received a review by BBB on IRC yesterday. Pushed.
It's kind of hard to follow these changes and the function in general, but
after looking at the function and this patch a few times it seems alright
and simpler.
Alexander
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] pthread_frame: minor simplification to error handling
On 2017-03-30 00:00 +0200, wm4 wrote: > On Wed, 29 Mar 2017 22:25:48 +0200 > Alexander Strasser wrote: > > > On Mon, 27 Mar 2017 14:28:17 +0200 > > > wm4 wrote: > > > > > > > Get rid of the "ret" variable, and always use err. Report the packet as > > > > consumed if err is unset. This should be equivalent to the old code, > > > > which obviously required err=0 for p->result>=0 (and otherwise, > > > > p->result must have had the value err was last set to). The code block > > > > added by commit 32a5b631267 is also not needed anymore, because the new > > > > code strictly returns err if it's >=0. > > > > --- > > > > Not totally sure about this, but it seems to work out? I probably missed > > > > something obvious. > > > > --- > > > > libavcodec/pthread_frame.c | 19 +-- > > > > 1 file changed, 5 insertions(+), 14 deletions(-) > > > > > > > > diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c > > > > index b618be0bf5..36e4a8affa 100644 > > > > --- a/libavcodec/pthread_frame.c > > > > +++ b/libavcodec/pthread_frame.c > > > > @@ -468,7 +468,7 @@ int ff_thread_decode_frame(AVCodecContext *avctx, > > > > FrameThreadContext *fctx = avctx->internal->thread_ctx; > > > > int finished = fctx->next_finished; > > > > PerThreadContext *p; > > > > -int err, ret = 0; > > > > +int err; > > > > Wouldn't it have been more readable to keep ret instead of err? > > It actually always used "err" - ret was introduced by a recent commit. > err also seemed to be used more often in the function. Right, I had found that when looking at the history of the file. But "err" was not used to hold a size at that time. > > Assigning a size to err looks a bit strange. I am not insisting on changing > > it now, just wanted to point this out. > > Actually I think the packet size could be removed, and the utils.c code > adjusted. Sorry, I cannot judge that from what I know. Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Ignore expired cookies
Hi!
On 2017-03-29 19:42 -0400, Micah Galizia wrote:
> I'm going to have to submit a v2 for this patch (hopefully soon) --
> this version only accomplishes half the job: not sending expired
> cookies. The change should also prevent storing them in the first
> place.
>
> Regardless, thanks for your help on this one.
I noticed one or two things while reading the patch. See below.
> On Sat, Mar 25, 2017 at 7:27 PM, Micah Galizia wrote:
> > Signed-off-by: Micah Galizia
> > ---
> > libavformat/http.c | 43 +++
> > 1 file changed, 39 insertions(+), 4 deletions(-)
> >
> > diff --git a/libavformat/http.c b/libavformat/http.c
> > index 293a8a7..53fae2a 100644
> > --- a/libavformat/http.c
> > +++ b/libavformat/http.c
> > @@ -29,6 +29,7 @@
> > #include "libavutil/avstring.h"
> > #include "libavutil/opt.h"
> > #include "libavutil/time.h"
> > +#include "libavutil/parseutils.h"
> >
> > #include "avformat.h"
> > #include "http.h"
> > @@ -48,6 +49,8 @@
> > #define MAX_REDIRECTS 8
> > #define HTTP_SINGLE 1
> > #define HTTP_MUTLI2
> > +#define MAX_EXPIRY19
> > +#define WHITESPACES " \n\t\r"
> > typedef enum {
> > LOWER_PROTO,
> > READ_HEADERS,
> > @@ -877,15 +880,20 @@ static int get_cookies(HTTPContext *s, char
> > **cookies, const char *path,
> >
> > *cookies = NULL;
> > while ((cookie = av_strtok(set_cookies, "\n", &next))) {
> > -int domain_offset = 0;
> > +int domain_offset = 0, expired = 0;
> > char *param, *next_param, *cdomain = NULL, *cpath = NULL, *cvalue
> > = NULL;
> > +char exp_buf[MAX_EXPIRY];
> > set_cookies = NULL;
> >
> > // store the cookie in a dict in case it is updated in the response
> > if (parse_cookie(s, cookie, &s->cookie_dict))
> > av_log(s, AV_LOG_WARNING, "Unable to parse '%s'\n", cookie);
> >
> > -while ((param = av_strtok(cookie, "; ", &next_param))) {
> > +while ((param = av_strtok(cookie, ";", &next_param))) {
> > +
> > +// move past any leading whitespace
> > +param += strspn(param, WHITESPACES);
> > +
> > if (cookie) {
> > // first key-value pair is the actual cookie value
> > cvalue = av_strdup(param);
> > @@ -899,6 +907,33 @@ static int get_cookies(HTTPContext *s, char **cookies,
> > const char *path,
> > int leading_dot = (param[7] == '.');
> > av_free(cdomain);
> > cdomain = av_strdup(¶m[7+leading_dot]);
> > +} else if (!av_strncasecmp("expires=", param, 8)) {
> > +int i, j, exp_len, exp_buf_len = MAX_EXPIRY-1;
> > +struct tm tm_buf = {0};
> > +char *expiry = ¶m[8];
> > +
> > +// strip off any punctuation or whitespace
> > +exp_len = strlen(expiry);
> > +for (i = 0, j = 0; i < exp_len && j < exp_buf_len; i++) {
> > +if ((expiry[i] >= '0' && expiry[i] <= '9') ||
> > +(expiry[i] >= 'A' && expiry[i] <= 'Z') ||
> > +(expiry[i] >= 'a' && expiry[i] <= 'z')) {
> > +exp_buf[j] = expiry[i];
> > +j++;
> > +}
> > +}
If expiry is zero terminated and you are going through it one byte at a time,
you could omit the strlen call and just check if expiry[i] is non zero.
It's maybe the more common idiom too.
> > +exp_buf[j] = '\0';
> > +expiry = exp_buf;
> > +
> > +// move the string beyond the day of week
> > +while ((*expiry < '0' || *expiry > '9') && *expiry != '\0')
> > +expiry++;
> > +
> > +if (av_small_strptime(expiry, "%d%b%Y%H%M%S", &tm_buf)) {
> > +time_t now = av_gettime() / 100;
> > +if (av_timegm(&tm_buf) < now)
> > +expired = 1;
> > +}
The patch seems a bit long for what it achieves. I don't really have any
better idea for now. One thing that came to mind: you might be able to
merge the skipping-day-of-week loop into the first loop that writes the
stripped down date string into the buffer.
Alexander
> > } else {
> > // ignore unknown attributes
> > }
> > @@ -907,9 +942,9 @@ static int get_cookies(HTTPContext *s, char **cookies,
> > const char *path,
> > cdomain = av_strdup(domain);
> >
> > // ensure all of the necessary values are valid
> > -if (!cdomain || !cpath || !cvalue) {
> > +if (expired || !cdomain || !cpath || !cvalue ) {
> > av_log(s, AV_LOG_WARNING,
> > - "Invalid cookie found, no value, path or domain
> > specified\n");
> > + "Invalid cookie found, expired or no value, path or
> > domain specified\n");
> >
Re: [FFmpeg-devel] [PATCH] Ignore expired cookies
Hi Nicolas! On 2017-03-30 22:12 +0200, Nicolas George wrote: > Le decadi 10 germinal, an CCXXV, Alexander Strasser a écrit : > > If expiry is zero terminated and you are going through it one byte at a > > time, > > you could omit the strlen call and just check if expiry[i] is non zero. > > > > It's maybe the more common idiom too. > > On the other hand, code that does not need a 0-terminated string is more > generic. This code depends already on a 0-terminated string. Otherwise the call of strlen before would be wrong. Also the later loop is also written against a 0-terminated string. IMHO for this patch it's not really important to avoid relying on 0-terminated string. > I am thinking of the ideas I posted a few months ago about reworking the > options system to avoid escaping hell. Parsers for various types must be > able to work in the middle of strings with foreign delimiters. Code that > is already capable of working in the middle of a string would be easier > to integrate. > > Of course, all this is in the far future. I will not insist on all new > code to be able to work like that, but I would like it nonetheless. And > if it is already written that way, let us keep it. I am not in general against getting rid of 0-terminated string in some places. I often thought of this too. > All in all, 0-terminated strings were a terrible terrible idea. Yes, they probably still are a bad idea. Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil/avstring: add av_strreplace API into avstring
On 2017-04-01 13:12 +0200, wm4 wrote: > On Sat, 1 Apr 2017 10:01:15 +0200 > Nicolas George wrote: [...] > > Therefore, it was not acceptable as is and should not have been pushed. > > > > Pushing while leaving only half a day to answer this was UNACCEPTABLE. > > Do not ever do it again please. > > Why get upset over a simple function being added? > > Lots of unreviewed patches or patches that were half-approved or not > really approved get pushed. That's how this project work. If you don't > like it, propose a rule change. I do not think a rule change is needed for this case at hand. While it is probably true that there were patches that didn't have enough review before they were pushed, it is not at all common, that patches which are the subject of an active discussion, as it was in this case, just get pushed. Additionally this patch adds public API which should be sufficiently discussed because reverting soon becomes difficult and involved. [...] Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat: Add max_streams option
Hi Michael!
On 2016-11-18 17:03 +0100, Michael Niedermayer wrote:
> This allows user apps to stop OOM due to excessive number of streams
> TODO: bump & docs
>
> Signed-off-by: Michael Niedermayer
> ---
> libavformat/avformat.h | 7 +++
> libavformat/options_table.h | 1 +
> libavformat/utils.c | 2 +-
> 3 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> index f9f4d72..c81a916 100644
> --- a/libavformat/avformat.h
> +++ b/libavformat/avformat.h
> @@ -1899,6 +1899,13 @@ typedef struct AVFormatContext {
> * - decoding: set by user through AVOptions (NO direct access)
> */
> char *protocol_blacklist;
> +
> +/**
> + * The maximum number of streams.
> + * - encoding: unused
> + * - decoding: set by user through AVOptions (NO direct access)
> + */
> +int max_streams;
> } AVFormatContext;
>
> int av_format_get_probe_score(const AVFormatContext *s);
> diff --git a/libavformat/options_table.h b/libavformat/options_table.h
> index 9d61d5a..d5448e5 100644
> --- a/libavformat/options_table.h
> +++ b/libavformat/options_table.h
> @@ -105,6 +105,7 @@ static const AVOption avformat_options[] = {
> {"format_whitelist", "List of demuxers that are allowed to be used",
> OFFSET(format_whitelist), AV_OPT_TYPE_STRING, { .str = NULL }, CHAR_MIN,
> CHAR_MAX, D },
> {"protocol_whitelist", "List of protocols that are allowed to be used",
> OFFSET(protocol_whitelist), AV_OPT_TYPE_STRING, { .str = NULL }, CHAR_MIN,
> CHAR_MAX, D },
> {"protocol_blacklist", "List of protocols that are not allowed to be used",
> OFFSET(protocol_blacklist), AV_OPT_TYPE_STRING, { .str = NULL }, CHAR_MIN,
> CHAR_MAX, D },
> +{"max_streams", "maximum number of streams", OFFSET(max_streams),
> AV_OPT_TYPE_INT, { .i64 = INT_MAX }, 0, INT_MAX, D },
> {NULL},
> };
>
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index 5664646..ded0b52 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -4210,7 +4210,7 @@ AVStream *avformat_new_stream(AVFormatContext *s, const
> AVCodec *c)
> int i;
> AVStream **streams;
>
> -if (s->nb_streams >= INT_MAX/sizeof(*streams))
> +if (s->nb_streams >= s->max_streams/sizeof(*streams))
> return NULL;
> streams = av_realloc_array(s->streams, s->nb_streams + 1,
> sizeof(*streams));
> if (!streams)
Maybe I am completely wrong, if so just tell me and sorry for the noise...
You name the option max_streams, but you compare it to nb_streams after
you divided it by size of a pointer.
I guess you change it that way so the default stays the same. I hope that
can be achieved in a different way.
Alexander
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Added the interface for the Turing codec
On 2016-11-19 14:29 +0100, Moritz Barsnick wrote: > On Fri, Nov 18, 2016 at 20:15:30 +0100, wm4 wrote: > > This lib has a really weird API... > > I can't judge the algorithms or the development direction and stuff - > I'm sure it's all state of the art. But the API sure gives me > goosebumps, and I don't mean the sexy feelgood type. > > Sure, it has all the flexibility, but also almost all the disadvantages > of calling an external program. This makes it look so much like a proof > of concept. I looked at the source for fun yesterday to find out what that was about. IMHO it is that the real encoder class takes C++ arguments (boost variables map) and they have a CLI program which also needs to create that variables map for the encoder. Thus it seems the quickest way to make a C interface was to use that argument parsing code for initializing the encoder. I hope it copes well with multiple definitions of the same option, like the last overwriting the previous ones. [...] Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [CLT2017] FFmpeg at Chemnitzer Linux-Tage
Hello all, like in the previous years we applied for a booth at 'Chemnitzer Linux-Tage' in Germany. Fortunately we were accepted again! The event will be next week end on 11th & 12th of March in Chemnitz. More information can be found here: https://chemnitzer.linux-tage.de/2017/en/ Currently registered as booth staff: Thilo Borgmann Carl Eugen Hoyos Thomas Volkert Alexander Strasser If you want to drop by or help out, it would be nice if you let us know about it here. I apologize for the late announcement this year. Best Regards, Alexander P.S. Slightly OT for this mail but still: If you have recommendations for open source events, you can add them on https://trac.ffmpeg.org/wiki/Conferences It should help remembering the dates and planning the yearly schedule. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [CLT2017] FFmpeg at Chemnitzer Linux-Tage
Hi Paul! On 2017-03-04 12:33 +0100, Paul B Mahol wrote: > On 3/4/17, Alexander Strasser wrote: > > > > like in the previous years we applied for a booth at 'Chemnitzer Linux-Tage' > > in Germany. Fortunately we were accepted again! > > > > The event will be next week end on 11th & 12th of March in Chemnitz. > > More information can be found here: > > > > https://chemnitzer.linux-tage.de/2017/en/ > > > > Currently registered as booth staff: > > > > Thilo Borgmann > > Carl Eugen Hoyos > > Thomas Volkert > > Alexander Strasser > > > > > > If you want to drop by or help out, it would be nice if you let us > > know about it here. > > What will be presented in FFmpeg booth? We will for sure show some filter graphs in action. Also there is a poster with basic ffmpeg command lines, to help us explaining FFmpeg usage to visitors and for showing some of the use cases for FFmpeg. In the last years I had a filter graph that split the video and generated a histogram of the pixel values and the wave form of the audio. At the end everything was output along with the original video (without audio). Another thing we traditionally do in Chemnitz is to encode video from a web cam and immediately decode with motion vector visualization. Both of this demos worked great to attract visitors and especially the former to steer up interest for libavfilter. Thomas posted both command lines on fb after last years event. We will have 2 Displays facing to the visitors. @all: If you have ideas for filters or features you think we can show case feel free to tell us. So we can try and see if it's feasible. [...] Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/2] lavf: Remove unnecessary escaping of ' in string literals
Signed-off-by: Alexander Strasser
---
libavformat/avio.c | 4 ++--
libavformat/utils.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/libavformat/avio.c b/libavformat/avio.c
index 62233a6..9020aa9 100644
--- a/libavformat/avio.c
+++ b/libavformat/avio.c
@@ -179,12 +179,12 @@ int ffurl_connect(URLContext *uc, AVDictionary **options)
(uc->protocol_blacklist && !strcmp(uc->protocol_blacklist,
e->value)));
if (uc->protocol_whitelist && av_match_list(uc->prot->name,
uc->protocol_whitelist, ',') <= 0) {
-av_log(uc, AV_LOG_ERROR, "Protocol not on whitelist \'%s\'!\n",
uc->protocol_whitelist);
+av_log(uc, AV_LOG_ERROR, "Protocol not on whitelist '%s'!\n",
uc->protocol_whitelist);
return AVERROR(EINVAL);
}
if (uc->protocol_blacklist && av_match_list(uc->prot->name,
uc->protocol_blacklist, ',') > 0) {
-av_log(uc, AV_LOG_ERROR, "Protocol blacklisted \'%s\'!\n",
uc->protocol_blacklist);
+av_log(uc, AV_LOG_ERROR, "Protocol blacklisted '%s'!\n",
uc->protocol_blacklist);
return AVERROR(EINVAL);
}
diff --git a/libavformat/utils.c b/libavformat/utils.c
index 37d7024..9c8287f 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -374,7 +374,7 @@ int av_demuxer_open(AVFormatContext *ic) {
int err;
if (ic->format_whitelist && av_match_list(ic->iformat->name,
ic->format_whitelist, ',') <= 0) {
-av_log(ic, AV_LOG_ERROR, "Format not on whitelist \'%s\'\n",
ic->format_whitelist);
+av_log(ic, AV_LOG_ERROR, "Format not on whitelist '%s'\n",
ic->format_whitelist);
return AVERROR(EINVAL);
}
@@ -554,7 +554,7 @@ int avformat_open_input(AVFormatContext **ps, const char
*filename,
}
if (s->format_whitelist && av_match_list(s->iformat->name,
s->format_whitelist, ',') <= 0) {
-av_log(s, AV_LOG_ERROR, "Format not on whitelist \'%s\'\n",
s->format_whitelist);
+av_log(s, AV_LOG_ERROR, "Format not on whitelist '%s'\n",
s->format_whitelist);
ret = AVERROR(EINVAL);
goto fail;
}
--
2.7.4
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 2/2] lavf: Be more explicit in logging white/black list matches
The current form of the messages indicating matches in the white
or black lists seems to be a bit too much relying on context.
Make the messages more explicit.
This also matches the way codec white list errors are reported.
Signed-off-by: Alexander Strasser
---
libavformat/avio.c | 4 ++--
libavformat/utils.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/libavformat/avio.c b/libavformat/avio.c
index 9020aa9..0e29c61 100644
--- a/libavformat/avio.c
+++ b/libavformat/avio.c
@@ -179,12 +179,12 @@ int ffurl_connect(URLContext *uc, AVDictionary **options)
(uc->protocol_blacklist && !strcmp(uc->protocol_blacklist,
e->value)));
if (uc->protocol_whitelist && av_match_list(uc->prot->name,
uc->protocol_whitelist, ',') <= 0) {
-av_log(uc, AV_LOG_ERROR, "Protocol not on whitelist '%s'!\n",
uc->protocol_whitelist);
+av_log(uc, AV_LOG_ERROR, "Protocol (%s) not on whitelist '%s'!\n",
uc->prot->name, uc->protocol_whitelist);
return AVERROR(EINVAL);
}
if (uc->protocol_blacklist && av_match_list(uc->prot->name,
uc->protocol_blacklist, ',') > 0) {
-av_log(uc, AV_LOG_ERROR, "Protocol blacklisted '%s'!\n",
uc->protocol_blacklist);
+av_log(uc, AV_LOG_ERROR, "Protocol (%s) on blacklist '%s'!\n",
uc->prot->name, uc->protocol_blacklist);
return AVERROR(EINVAL);
}
diff --git a/libavformat/utils.c b/libavformat/utils.c
index 9c8287f..542357e 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -374,7 +374,7 @@ int av_demuxer_open(AVFormatContext *ic) {
int err;
if (ic->format_whitelist && av_match_list(ic->iformat->name,
ic->format_whitelist, ',') <= 0) {
-av_log(ic, AV_LOG_ERROR, "Format not on whitelist '%s'\n",
ic->format_whitelist);
+av_log(ic, AV_LOG_ERROR, "Format (%s) not on whitelist '%s'\n",
ic->iformat->name, ic->format_whitelist);
return AVERROR(EINVAL);
}
@@ -554,7 +554,7 @@ int avformat_open_input(AVFormatContext **ps, const char
*filename,
}
if (s->format_whitelist && av_match_list(s->iformat->name,
s->format_whitelist, ',') <= 0) {
-av_log(s, AV_LOG_ERROR, "Format not on whitelist '%s'\n",
s->format_whitelist);
+av_log(s, AV_LOG_ERROR, "Format (%s) not on whitelist '%s'\n",
s->iformat->name, s->format_whitelist);
ret = AVERROR(EINVAL);
goto fail;
}
--
2.7.4
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 0/2] lavf: Error reporting when matching in black/white lists
Hi, while investigating this issue filed at Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=857369 I found that the error messages can easily be misunderstood. Though in that particular case the first problem is the placement of the whitelist option in front of the output. Alexander Strasser (2): lavf: Remove unnecessary escaping of ' in string literals lavf: Be more explicit in logging white/black list matches libavformat/avio.c | 4 ++-- libavformat/utils.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) -- 2.7.4 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 0/2] lavf: Error reporting when matching in black/white lists
On 2017-03-11 18:54 +0100, Alexander Strasser wrote: > while investigating this issue filed at Debian: > > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=857369 > > I found that the error messages can easily be misunderstood. > > Though in that particular case the first problem is the placement > of the whitelist option in front of the output. > > > Alexander Strasser (2): > lavf: Remove unnecessary escaping of ' in string literals > lavf: Be more explicit in logging white/black list matches > > libavformat/avio.c | 4 ++-- > libavformat/utils.c | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) Patch set pushed. Thanks for having a look, Michael. Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat/hlsenc: fix duration wrong when no pkt duration
Hi!
On 2017-03-13 17:12 +0800, Steven Liu wrote:
> when cannot get pkt duration, hlsenc segments duration will
> be set to 0, this patch can fix it.
>
> Signed-off-by: Steven Liu
> ---
> libavformat/hlsenc.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> index 5df2514..d6f0631 100644
> --- a/libavformat/hlsenc.c
> +++ b/libavformat/hlsenc.c
> @@ -1354,7 +1354,12 @@ static int hls_write_packet(AVFormatContext *s,
> AVPacket *pkt)
> * st->time_base.num /
> st->time_base.den;
> hls->dpp = (double)(pkt->duration) * st->time_base.num /
> st->time_base.den;
> } else {
> -hls->duration += (double)(pkt->duration) * st->time_base.num /
> st->time_base.den;
> +if (pkt->duration) {
> +hls->duration += (double)(pkt->duration) * st->time_base.num
> / st->time_base.den;
> +} else {
> +av_log(s, AV_LOG_WARNING, "pkt->duration = 0, maybe the hls
> segment duration will not precise\n");
^
There is a word missing in the log message: "be"
For a warning it might have been better to use more natural language and to
convey a bit more information:
"Packet duration is 0. Trying to compensate. Segment duration may not be
accurate.\n"
> +hls->duration = (double)(pkt->pts - hls->end_pts) *
> st->time_base.num / st->time_base.den;
> +}
> }
>
> }
I am sorry to comment after you pushed already. Anyway my remarks are only
cosmetics and not so important. I did not investigate the issue at all.
Thank you,
Alexander
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] avcodec/vp56: Require 1 undamaged frame for 5 damaged frames for concealment to be used
Hi!
This is not a review, just a comment you can address in case you push this.
On 2017-03-15 04:12 +0100, Michael Niedermayer wrote:
> Fixes timeout with 847/clusterfuzz-testcase-5291877358108672
> Fixes timeout with 850/clusterfuzz-testcase-5721296509861888
>
> This likely will need to be tweaked
>
> Found-by: continuous fuzzing process
> https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
> Signed-off-by: Michael Niedermayer
> ---
> libavcodec/vp56.c | 10 +++---
> libavcodec/vp56.h | 1 +
> 2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/libavcodec/vp56.c b/libavcodec/vp56.c
> index 9d4162bb96..b4ee760080 100644
> --- a/libavcodec/vp56.c
> +++ b/libavcodec/vp56.c
> @@ -508,6 +508,7 @@ static int vp56_size_changed(VP56Context *s)
> s->plane_height[1] = s->plane_height[2] = avctx->coded_height/2;
>
> s->have_undamaged_frame = 0;
> +s->damaged_frame_count = 0;
>
> for (i=0; i<4; i++)
> s->stride[i] = s->flip * s->frames[VP56_FRAME_CURRENT]->linesize[i];
> @@ -712,8 +713,9 @@ static int ff_vp56_decode_mbs(AVCodecContext *avctx, void
> *data,
> int ret = vp56_decode_mb(s, mb_row, mb_col, is_alpha);
> if (ret < 0) {
> damaged = 1;
> -if (!s->have_undamaged_frame) {
> +if (5*s->have_undamaged_frame <= s->damaged_frame_count)
> {
> s->discard_frame = 1;
> +s->damaged_frame_count ++;
If I am not mistaken, the space before ++ is not needed and not usual for
FFmpeg's code base.
> return AVERROR_INVALIDDATA;
> }
> }
> @@ -733,8 +735,10 @@ static int ff_vp56_decode_mbs(AVCodecContext *avctx,
> void *data,
> }
> }
>
> -if (!damaged)
> -s->have_undamaged_frame = 1;
> +if (!damaged) {
> +s->have_undamaged_frame ++;
> +} else
> +s->damaged_frame_count ++;
Same.
>
> next:
> if (p->key_frame || s->golden_frame) {
> diff --git a/libavcodec/vp56.h b/libavcodec/vp56.h
> index c049399df8..37837d0c7b 100644
> --- a/libavcodec/vp56.h
> +++ b/libavcodec/vp56.h
> @@ -206,6 +206,7 @@ struct vp56_context {
>
> int have_undamaged_frame;
> int discard_frame;
> +int damaged_frame_count;
> };
>
>
> --
> 2.11.0
Alexander
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 01/16] doc/filters: document the unstability of the shorthand options notation.
Hi all,
sorry for jumping into the discussion late. I think it is very
important though. Please pardon me if I missed anything from
the previous discussions on this topic.
On 2017-08-11 23:34 +0200, Nicolas George wrote:
> Le quartidi 24 thermidor, an CCXXV, Clement Boesch a écrit :
[...]
> > BTW, now that I think about it, how about this:
> >
> > We add a flag to AVOption such as AV_OPT_FLAG_FILTER_STABLE_SHORTHAND and
> > flag all the options that we believe won't need to move in the future and
> > for which we can always rely on (typically x and y in overlay, or w and h
> > in scale).
> >
> > Then we simply warn the user for every shorthand use of other options
> > ("using short-hand form with '' may cause issue in the future as
> > it could be swapped around").
Besides AV_OPT_FLAG_FILTER_STABLE_SHORTHAND being an awfully long name
for it, I think it is a good idea.
Maybe even explicitly marking the options that can be used without name
and have a stable order and not allowing the others to be used without
name would be even better. Documentation should clearly indicate for
which options the name could be omitted. I guess we would have to come
up with a clean notation for that, but that shouldn't be too hard.
> > That way, we:
> >
> > - give clear visibility of the instability of (some of) the shorthands
> > - affect only marginal uses (that is, late options because we are
> > generally going to flag the few first ones)
> > - gain flexibility to indeed swap around the options at will in the future
> > (we initially wait for a release or two such that the users gets aware
> > of their potential misuses with the introduced warning)
> > - keep shorthands useful for the most essential cases
> > - provide trust on the current use of shorthands.
>
> It looks nice on principle, but I think you forget one consideration
> about convenience: learning which option is stable and which is not
> requires more effort than just putting the names of the options always.
> So as soon as we acknowledge that some options are not stable with
> shorthand, it becomes more convenient to use the names, and any
> mitigation scheme that can be implemented will just be mostly unused.
I do not think so. For many use cases *not* using named options will
clearly be more attractive, e.g. only one stupid and simple example is
vf fps.
I think if it is not allowed (like I suggested above) to give some
arguments with the shorthand notation, this will support the use cases
where using the shorthand notation is most useful and convenient.
> Think of it that way: if we had designed the filters with an
> AVOption-based system from the start instead of going for a simple
> string and a custom parser, same as every time, would we have
> implemented the shorthand system? No, we would just have used the
> standard key=value:key=value... parser. The shorthand system only exists
> because we thought we could get away with parsing the filters options
> with sscanf(opt_str, "%d:%d", &w, &h) and we did not have the
> clairvoyance to make a clean break when switching to AVOption.
To me this reads like a huge over simplification, though it is
partially correct. But I strongly believe the shorthand notation
was always much more than a side effect of choosing the wrong
way to implement things.
Alexander
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [FFmpeg-cvslog] avcodec/pngdec: Fix () placement
On 2017-08-22 17:23 +, Michael Niedermayer wrote: > ffmpeg | branch: master | Michael Niedermayer | Tue > Aug 22 18:36:26 2017 +0200| [a2e444d5bb2e3115d3afcc0cca9d1506c90436a2] | > committer: Michael Niedermayer > > avcodec/pngdec: Fix () placement > > Signed-off-by: Michael Niedermayer > > > http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=a2e444d5bb2e3115d3afcc0cca9d1506c90436a2 > --- > > libavcodec/pngdec.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c > index 4fc1c5a062..dce8faf168 100644 > --- a/libavcodec/pngdec.c > +++ b/libavcodec/pngdec.c > @@ -858,7 +858,7 @@ static int decode_iccp_chunk(PNGDecContext *s, int > length, AVFrame *f) > > length = FFMAX(length - 1, 0); > > -if ((ret = decode_zbuf(&bp, s->gb.buffer, s->gb.buffer + length) < 0)) > +if ((ret = decode_zbuf(&bp, s->gb.buffer, s->gb.buffer + length)) < 0) IMHO another reason not to do the assignment and the comparison on the same line inside the if-condition in C. I mean you need the extra parens and if you are not careful enough you will just get it silently wrong like it was above. Probably we found enough of such errors to discourage that style in FFmpeg? > return ret; > > av_bprint_finalize(&bp, (char **)&data); Alexander ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] swresample: Add AVFrame based API
On 2014-08-11 21:42 +0200, Michael Niedermayer wrote: > On Mon, Aug 11, 2014 at 09:25:20AM +0100, Kieran Kunhya wrote: > > On 11 August 2014 00:49, Michael Niedermayer wrote: > > > From: Luca Barbato > > > > Clearly this is Luca's patch but rewritten by you for swr so you > > should say "Based on patch from Luca for avresample" or similar. > > its copy and pasted from lucas work and adapted where needed. > I would not call that rewritten. rewritten is if you start with an > empty file. > luca has certainly done the main work on this and i thus would like > to give him the majority of the credit for the work > but if he preferrs i certainly can also put my name in the author > field and add a "Based on commit ... by Luca ..." > > CCing luca > In absence of a reply i will follow kierans suggestion > > > > IANAL but this is quite dubious from a copyright perspective. Whoever ends up in the author field the commit message body should mention that you adapted Luca's work for libswresample. Just for clarity. Alexander pgpHnTg39aTqf.pgp Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] libavformat/ftp: Do not leak memory in routine ftp_features
Should fix CID1231988 (RESOURCE_LEAK) Signed-off-by: Alexander Strasser --- WARNING: Sorry, I only compile-tested so far. There is one remaining thing, I am not sure of: It looks like the variable feet could be uninitialized at the point av_freep is called. I think it cannot, but I had to follow quite some hops down the call hierarchy. I see two possibilities for taking care of that: 1) explicitly initialize it with NULL (could be merged with declaration) 2) someone else goes all the way down and if he comes to the same conclusion as I did, we add a comment stating that to ftp_send_command Alexander libavformat/ftp.c | 1 + 1 file changed, 1 insertion(+) diff --git a/libavformat/ftp.c b/libavformat/ftp.c index 9ee9b16..4c071bf 100644 --- a/libavformat/ftp.c +++ b/libavformat/ftp.c @@ -450,6 +450,7 @@ static int ftp_features(FTPContext *s) if (av_stristr(feat, "UTF8")) ftp_send_command(s, enable_utf8_command, opts_codes, NULL); } +av_freep(&feat); return 0; } -- pgpzqAUExDNSw.pgp Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] libavformat/ftp: Do not leak memory in routine ftp_features
On 2014-08-20 01:25 +0200, Alexander Strasser wrote: > Should fix CID1231988 (RESOURCE_LEAK) > > Signed-off-by: Alexander Strasser > --- > > WARNING: Sorry, I only compile-tested so far. > > There is one remaining thing, I am not sure of: > It looks like the variable feet could be uninitialized > at the point av_freep is called. I think it cannot, but > I had to follow quite some hops down the call hierarchy. > > I see two possibilities for taking care of that: > > 1) explicitly initialize it with NULL (could be merged with declaration) > 2) someone else goes all the way down and if he comes to the same conclusion > as I did, we add a comment stating that to ftp_send_command After sleeping over it I think I am wrong about 2 because ftp_send_command may early return and in those cases leave feat uninitialized. So here is some more: 3) Examine all places ftp_send_command, ftp_status is called and do what described for this instance in point 1 4) Change ftp_send_command and ftp_status to make sure the out parameter is always valid or at least NULL after a call Doing 4 seem best to me ATM but maybe I missed some things again :( Alexander > libavformat/ftp.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/libavformat/ftp.c b/libavformat/ftp.c > index 9ee9b16..4c071bf 100644 > --- a/libavformat/ftp.c > +++ b/libavformat/ftp.c > @@ -450,6 +450,7 @@ static int ftp_features(FTPContext *s) > if (av_stristr(feat, "UTF8")) > ftp_send_command(s, enable_utf8_command, opts_codes, NULL); > } > +av_freep(&feat); > return 0; > } > > -- pgpXz2NArbwef.pgp Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH v2] libavformat/ftp: Do not leak memory in routine ftp_features
Setting the pointer to NULL inside both ftp_send_command
and ftp_features is redundant. Generally always setting to
NULL in ftp_send_command seems safer, but throughout the file
that parameter was always passed initialized. So I do it here
too for consistency.
Should fix CID1231988 (RESOURCE_LEAK)
Signed-off-by: Alexander Strasser
---
I tested with valgrind and the mem leak goes away. About the
rest it would be good if someone could have a 2nd look.
And maybe Lukasz can comment if he finds some time because he
maintains this code.
libavformat/ftp.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/libavformat/ftp.c b/libavformat/ftp.c
index 9ee9b16..7faf4a5 100644
--- a/libavformat/ftp.c
+++ b/libavformat/ftp.c
@@ -183,6 +183,9 @@ static int ftp_send_command(FTPContext *s, const char
*command,
{
int err;
+if (response)
+*response = NULL;
+
if ((err = ffurl_write(s->conn_control, command, strlen(command))) < 0)
return err;
if (!err)
@@ -444,12 +447,14 @@ static int ftp_features(FTPContext *s)
static const char *enable_utf8_command = "OPTS UTF8 ON\r\n";
static const int feat_codes[] = {211, 0};
static const int opts_codes[] = {200, 451};
-char *feat;
+char *feat = NULL;
if (ftp_send_command(s, feat_command, feat_codes, &feat) == 211) {
if (av_stristr(feat, "UTF8"))
ftp_send_command(s, enable_utf8_command, opts_codes, NULL);
}
+av_freep(&feat);
+
return 0;
}
--
pgpPp3z4D5DcF.pgp
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] libavformat/ftp: Do not leak memory in routine ftp_features
On 2014-08-20 06:27 +0200, Alexander Strasser wrote: > On 2014-08-20 01:25 +0200, Alexander Strasser wrote: > > Should fix CID1231988 (RESOURCE_LEAK) > > > > Signed-off-by: Alexander Strasser > > --- > > > > WARNING: Sorry, I only compile-tested so far. > > > > There is one remaining thing, I am not sure of: > > It looks like the variable feet could be uninitialized > > at the point av_freep is called. I think it cannot, but > > I had to follow quite some hops down the call hierarchy. > > > > I see two possibilities for taking care of that: > > > > 1) explicitly initialize it with NULL (could be merged with declaration) > > 2) someone else goes all the way down and if he comes to the same > > conclusion > > as I did, we add a comment stating that to ftp_send_command > >After sleeping over it I think I am wrong about 2 because > ftp_send_command may early return and in those cases leave feat > uninitialized. > >So here is some more: > > 3) Examine all places ftp_send_command, ftp_status is called > and do what described for this instance in point 1 > 4) Change ftp_send_command and ftp_status to make sure the > out parameter is always valid or at least NULL after a call > > Doing 4 seem best to me ATM but maybe I missed some things again :( Dropped. I sent v2 which combines points 1, 3 and 4. https://ffmpeg.org/pipermail/ffmpeg-devel/2014-August/161668.html Subject: [FFmpeg-devel] [PATCH v2] libavformat/ftp: Do not leak memory in routine ftp_features Message-ID: Alexander pgpihp_luxi5_.pgp Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH v2] libavformat/ftp: Do not leak memory in routine ftp_features
On 2014-08-21 23:02 +0200, Alexander Strasser wrote:
> Setting the pointer to NULL inside both ftp_send_command
> and ftp_features is redundant. Generally always setting to
> NULL in ftp_send_command seems safer, but throughout the file
> that parameter was always passed initialized. So I do it here
> too for consistency.
>
> Should fix CID1231988 (RESOURCE_LEAK)
>
> Signed-off-by: Alexander Strasser
> ---
>
> I tested with valgrind and the mem leak goes away. About the
> rest it would be good if someone could have a 2nd look.
>
> And maybe Lukasz can comment if he finds some time because he
> maintains this code.
Received an OK from Lukasz in private.
Pushed.
Thanks for having a look Lukasz,
Alexander
> libavformat/ftp.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/libavformat/ftp.c b/libavformat/ftp.c
> index 9ee9b16..7faf4a5 100644
> --- a/libavformat/ftp.c
> +++ b/libavformat/ftp.c
> @@ -183,6 +183,9 @@ static int ftp_send_command(FTPContext *s, const char
> *command,
> {
> int err;
>
> +if (response)
> +*response = NULL;
> +
> if ((err = ffurl_write(s->conn_control, command, strlen(command))) < 0)
> return err;
> if (!err)
> @@ -444,12 +447,14 @@ static int ftp_features(FTPContext *s)
> static const char *enable_utf8_command = "OPTS UTF8 ON\r\n";
> static const int feat_codes[] = {211, 0};
> static const int opts_codes[] = {200, 451};
> -char *feat;
> +char *feat = NULL;
>
> if (ftp_send_command(s, feat_command, feat_codes, &feat) == 211) {
> if (av_stristr(feat, "UTF8"))
> ftp_send_command(s, enable_utf8_command, opts_codes, NULL);
> }
> +av_freep(&feat);
> +
> return 0;
> }
>
> --
pgpKNfoNH7gWB.pgp
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Include content of the news article in the website RSS
Hi! On 2014-05-23 00:28 +0200, Alexander Strasser wrote: > On 2014-05-11 01:37 +0200, Gerion Entrup wrote: > > Am Donnerstag 01 Mai 2014, 23:48:29 schrieb Alexander Strasser: > > > alternative. What do you and/or others think of it? > > > > > > Besides validation I couldn't yet properly test what feed > > > readers make of it. So please forgive me if it causes obvious > > > problems with your reader. > > (As said offline already) My reader works with it. My patch still works I think. I would like to commit it, though it won't be flexible enough to add much other stuff that needs more elaborate logic. I want to apply it, because it is a neat feature to have now. Any objections to this? In the not-so-short term I intend to widen security checks for the repo on the server to other files than just the Makefile. I already started this work, but need to fix one known problem before I can put it to work on the server. > I sent a new series for this: > > Subject: [PATCH 0/5] web: Extend RSS When what I described in the above paragraph is online, I could push the better solution that I quoted above. Alexander > > It would be nice to set the > > pubtime as well (an example: Wed, 07 May 2014 18:28:18 > > CEST). > > This command should work more less to build the date: > > date '+%a, %d %b %Y %T %Z' -d parsed_date > > (similar to date '+%c' btw) you have to keep in mind the locale, too. > > Good point. Actually I think I need to check where the time > formatting code in my new patch is getting the locale from... > > > See here > > (http://cyber.law.harvard.edu/rss/rss.html#optionalChannelElements) > > for the spec. > > [...] pgppqJ6bGthd3.pgp Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [libav-devel] Common mailing-list for API evolutions
On 2014-08-28 18:58 +0200, Anton Khirnov wrote: > On Sun, 24 Aug 2014 00:28:56 +0200, =?utf-8?B?Q2zDqW1lbnQgQsWTc2No?= > wrote: > > Kieran suggested tonight on #ffmpeg-devel to have a common mailing-list > > between the two projects to start communicating again in sane terms. > > > > The proposition would be a mailing-list where the 2 projects would send > > the patches that will make API evolutions. So the projects can continue to > > drop or add codecs & filters without caring about the other, but will try > > to communicate more about the API, for the sake of our common users. > > > > At first, I suggest that won't engage anything from any of the two > > projects (so we don't end up in a stalled states such as one project > > trying to block the other), but it could be seen as a way to introduce > > some common technical ground. > > > > What do you think? > > While some kind of non-hostile coexistence or even cooperation is desirable > and > might even be possible, I have large doubts that this specific approach can > work. > > First, some of your project's developers (most importantly your leader) > are being actively hostile to our project. That includes spreading FUD about > us > all over the internet, stalking our new contributors, etc. I do not think any > kind of cooperation can work while this crap goes on. > > Second, how do you propose this arrangement will actually function? As you > probably know, I see many of the API additions done in your project as ugly > hacks, and would be strongly opposed to having them in our tree in their > current > form. Conversely, some API changes done in Libav were AFAIK rejected by your > leader. So -- what happens when one side proposes a change that the other side > fundamentally disagrees with. > And furthermore -- what would ensure that the code actually gets pushed to > both > trees. Because otherwise there really is no point to this. Please read what you wrote again; it is almost completely hostile towards FFmpeg... Alexander pgpdqlzs924XR.pgp Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Include content of the news article in the website RSS
On 2014-08-28 07:23 -0700, Timothy Gu wrote: > On Thu, Aug 28, 2014 at 7:26 AM, Alexander Strasser wrote: > > Hi! > > > > On 2014-05-23 00:28 +0200, Alexander Strasser wrote: > >> On 2014-05-11 01:37 +0200, Gerion Entrup wrote: > >> > Am Donnerstag 01 Mai 2014, 23:48:29 schrieb Alexander Strasser: > >> > > alternative. What do you and/or others think of it? > >> > > > >> > > Besides validation I couldn't yet properly test what feed > >> > > readers make of it. So please forgive me if it causes obvious > >> > > problems with your reader. > >> > (As said offline already) My reader works with it. > > > > My patch still works I think. I would like to commit it, > > though it won't be flexible enough to add much other stuff > > that needs more elaborate logic. I want to apply it, because > > it is a neat feature to have now. > > Good to hear you are still working on this. > > > Any objections to this? > > LGTM from me, assuming it is tested and works. I slightly prefer your > patch over Gerion's, but both are OK. Thanks. Pushed mine. Alexander > > > > In the not-so-short term I intend to widen security checks > > for the repo on the server to other files than just the Makefile. > > I already started this work, but need to fix one known problem > > before I can put it to work on the server. > > Yes, that would be much better. The awk and sed hack is getting too > cumbersome. Or we could just change the server host. pgp8gLTQw62yO.pgp Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat/img2dec: fix glob pattern detection.
Hi Benoit,
thank you for investigating this issue and sending a patch.
On 2014-09-22 12:30 +0200, Benoit Fouet wrote:
> The is_glob() function was not working with unescaped glob patterns,
> which is the way only glob_sequence (which is deprecated) works.
> Fixes ticket #3948
> ---
> libavformat/img2dec.c | 14 +-
> 1 file changed, 1 insertion(+), 13 deletions(-)
>
> diff --git a/libavformat/img2dec.c b/libavformat/img2dec.c
> index a21429f..64ebc31 100644
> --- a/libavformat/img2dec.c
> +++ b/libavformat/img2dec.c
> @@ -75,19 +75,7 @@ static int infer_size(int *width_ptr, int *height_ptr, int
> size)
> static int is_glob(const char *path)
The way you want to change this function makes its semantics
unsuitable for the other place it is called. Actually it is not
the is_glob that is wrong here but the problem is the way the
demuxer selection changed since commit b3fd2b17 which fixed
ticket #3901.
Though I agree it would be desirable to have the command line
pointed out in ticket #3948 still working.
One way to do achieve it is to be more heuristic like you
did below.
> {
> #if HAVE_GLOB
> -size_t span = 0;
> -const char *p = path;
> -
> -while (p = strchr(p, '%')) {
> -if (*(++p) == '%') {
> -++p;
> -continue;
> -}
> -if (span = strspn(p, "*?[]{}"))
> -break;
> -}
> -/* Did we hit a glob char or get to the end? */
> -return span != 0;
> +return strspn(path, "%*?[]{}") != 0;
This seems wrong; it would only work for '*.png' but
not for './*.png' or 'dir/*.png'. or 'foo-*.png' etc.
Maybe something like this patch would be acceptable
(WARNING: only lightly tested):
diff --git a/libavformat/img2dec.c b/libavformat/img2dec.c
index 16bd699..aa7c2f6 100644
--- a/libavformat/img2dec.c
+++ b/libavformat/img2dec.c
@@ -160,7 +160,7 @@ static int img_read_probe(AVProbeData *p)
if (p->filename && ff_guess_image2_codec(p->filename)) {
if (av_filename_number_test(p->filename))
return AVPROBE_SCORE_MAX;
-else if (is_glob(p->filename))
+else if (p->filename[strcspn(p->filename, "*?{\0")]) // glob pattern?
return AVPROBE_SCORE_MAX;
else if (p->buf_size == 0)
return 0;
[...]
Alexander
pgpwJheujtWq8.pgp
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat/img2dec: fix glob pattern detection.
On 2014-10-02 22:51 +0200, Alexander Strasser wrote:
[...]
> Maybe something like this patch would be acceptable
> (WARNING: only lightly tested):
>
> diff --git a/libavformat/img2dec.c b/libavformat/img2dec.c
> index 16bd699..aa7c2f6 100644
> --- a/libavformat/img2dec.c
> +++ b/libavformat/img2dec.c
> @@ -160,7 +160,7 @@ static int img_read_probe(AVProbeData *p)
> if (p->filename && ff_guess_image2_codec(p->filename)) {
> if (av_filename_number_test(p->filename))
> return AVPROBE_SCORE_MAX;
> -else if (is_glob(p->filename))
> +else if (p->filename[strcspn(p->filename, "*?{\0")]) // glob pattern?
I am sorry that superflous NUL slipped in there :( Please ignore it.
For as to why I did not include "[" in the list of glob chars, is
because I think it is too common in file and directory names to use
as a heuristic to favour image2 demuxer.
> return AVPROBE_SCORE_MAX;
> else if (p->buf_size == 0)
> return 0;
[...]
Alexander
pgpIhQQ7zlGf4.pgp
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avformat/img2dec: Attempt to detect non-escaped glob patterns too (-pattern_type glob)
Fixes ticket #3948
Based-on-patch-by: Michael Niedermayer
Signed-off-by: Alexander Strasser
---
- fixes the \0 typo in the string literal
- cosmetic change to the score comment
- remove the -pattern_type comment because as the
options matching system works now it would not
be the "ideal" solution I think
(I tried to explain why in #ffmpeg-devel)
Alexander
libavformat/img2dec.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/libavformat/img2dec.c b/libavformat/img2dec.c
index 5aac555..a06933d 100644
--- a/libavformat/img2dec.c
+++ b/libavformat/img2dec.c
@@ -162,6 +162,8 @@ static int img_read_probe(AVProbeData *p)
return AVPROBE_SCORE_MAX;
else if (is_glob(p->filename))
return AVPROBE_SCORE_MAX;
+else if (p->filename[strcspn(p->filename, "*?{")]) // probably PT_GLOB
+return AVPROBE_SCORE_EXTENSION + 2; // score choosen to be a tad
above the image pipes
else if (p->buf_size == 0)
return 0;
else if (av_match_ext(p->filename, "raw") || av_match_ext(p->filename,
"gif"))
--
pgpr8jirolNMK.pgp
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [RFC][PATCH] web: News about FFmpeg in Debian
Try to shortly summarize where we are now. Signed-off-by: Alexander Strasser --- This is all RFC as marked in the subject. Please give me some feed back! It would also be nice if someone could test the patch, I do not have the tools at hand ATM. src/index | 16 1 file changed, 16 insertions(+) diff --git a/src/index b/src/index index 8d3f541..afc276b 100644 --- a/src/index +++ b/src/index @@ -36,6 +36,22 @@ News + October 6, 2014, FFmpeg is in Debian unstable again + +We wanted you to know there are +https://packages.debian.org/search?keywords=ffmpeg&searchon=sourcenames&suite=unstable§ion=main";> +FFmpeg packages in Debian unstable again. A big thank you +to all the people that made it possible. It has been anything but simple. + + +Unfortunately that was already the easy part of this news. The bad news is the packages probably won't +migrate to Debian testing in time for the upcoming realease. +https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=763148";>Read the argumentation over at Debian. + + +However things will come out in the end, we hope for your continued remarkable support! + + September 15, 2014, FFmpeg 2.4 We have made a new major release (2.4) -- pgpwqj5C_Efo3.pgp Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
