OpenCVのバグ(Pull Request)

OpenCVのバグ(trac)

この度、OpenCVへのPull Request がマージされたので、その時の記録とかを書いておきます。

Itroduction†

• First, always check the https://github.com/opencv/opencv/wiki/How_to_contribute

時系列†

testのfailに気づく†

• ことの始まりはODROID-C2を買って、その上でOpenCVのtestを走らせたことから
• 何故か、testの一部がFAILEDとなる
• 見てみると、hal_intrinというテストが失敗していた
• 具体的には、HW命令を使用して平方根(square root)を求めた結果と、std::sqrtの結果が一致しないというエラーだった

  /home/odroid/opencv-fork/modules/core/test/test_intrin.cpp:578: Failure
  Value of: resF[i]
    Actual: 11.054863929748535
  Expected: std::sqrt(data1[i]*data1[i] + data2[i]*data2[i])
  Which is: 11.054862976074219

• ODROID-C2は、純粋に64bitArmであり&note{raspberry-pi-3:話題を呼んだラズパイ3は、OSは32bit。ODROID-C2は64bitのUbuntu16.04が動くのが特徴};、このテストをfixしてみようと気づいた

エラーしている箇所を眺めてみる†

• 内容は、11.054862976074219になるべき結果が、
• なぜか、11.054863929748535になっているというエラー
• もともとも、HW命令を使った計算では、ニュートン法で&mimetex(\frac{1}{\sqrt{x}});を求めて、そこから&mimetex(\sqrt{x});を求めるアプローチ
• 反復することで、精度があがるので、まぁ、もう1回ぐらい計算を繰り返したら微妙な誤差も消えるだろう、と思って、試しに、もう1回繰り返してみた。

  #geshi(c++){{

      float32x4_t e = vrsqrteq_f32(x1);
      e = vmulq_f32(vrsqrtsq_f32(vmulq_f32(x1, e), e), e);
      e = vmulq_f32(vrsqrtsq_f32(vmulq_f32(x1, e), e), e);

1. e = vmulq_f32(vrsqrtsq_f32(vmulq_f32(x1, e), e), e);

       return v_float32x4(vmulq_f32(x.val, e));
   }

   }}

• こんな感じに、eに入る値がどんどん&mimetex(\frac{1}{\sqrt{x}});に近づいていくという寸法。
• ちなみに、ArmのNEON命令を使っているので、float4つ分が一気に計算される。
• ベクタの要素のうち1つが、計算結果がズレ過ぎている、としてFAILEDになっていた

修正案を実装してPR(Pull Request)する†

• 試しに実装したらテストのFAILEDがPASSした
• なので、1行だけの修正だし、これで良かれと思って、PRを投げることにした

How to contributeページを読む†

• ここからが大事なところ。OpenCVでは、How to contirbute&note{how-to-contribute-opencv:How to contirbute, 2022-05-20 18:53版, 2022-07-07閲覧};というページがある
• ここでは、PRを投げる前に読むべき大事な点がたくさん書かれているので、本当に読んだ方が良い
• Gitのお作法だけでなく、コーディングスタイルに関しても書かれている。
• そして、なるだけ、機械的なチェックをパスするように指導されている。その一部をここで紹介
• 注意 ここで書いている情報は2016-05-16時点の情報なので、これを読んでPRを投げる人は、必ずWikiで最新版の情報を参照して下さい

Githubのお作法†

• この節だけはOpenCV以外のプロジェクトにも共通する、gitのfork-mergeに関するお話&note{github-creating-pull-request:Creating a pull request - User Documentation, 2016-05-16閲覧};&note{github-fork-a-repo:Fork A Repo, 2016-05-16閲覧};
• 手順としては
  

1. opencv/opencv リポジトリを自身のリポジトリに fork する
2. 自身のリポジトリから、ローカルに clone する
3. ローカルでfixして、commitする
4. 自身のリポジトリにpushする
5. GithubのインタフェースからPull Request を作る

• という感じです。詳細はgithubの説明(リポジトリをforkする)githubの説明(Pull Requestを使う)当たりを参照して下さい

使用するブランチ†

• 以降の節はOpenCVに限った話
• OpenCVは2016年5月16日現在、3.0系列と2.4系列の2本の系列があります。
  • OpenCV2.4系列は既に「新機能の追加は行わない」とアナウンスされており、bug fixだけを取り込むこととなっています
  • OpenCV3.0系列がメインの開発対象となっており、新規機能はこちらに取り込むこととなっています
• なので、bug fix ならば2.4系列、もしくは両方に、新規機能の実装ならば3.0系列に、それぞれPRを投げることになります。
• 今回私が投げたPRはbug fixなので、本来は2.4系列にも投げるべきなのですが、そもそもhal_intrinがHALモジュール(Hardware Acceleration Layer)という、3.0から入った機能を使った話なので、今回はmaster(3.0系列)にPRを投げることにしました。

pre-commit hook†

• 前述の手順でローカルにリポジトリをcloneした後、作業をする前に、pre-commit hook を用意すると、みんな幸せになれます
• 具体的には、cloneしたリポジトリの .git/hooks/pre-commit.sample というファイルを、 .git/hooks/pre-commit にリネームします。
• この.gitディレクトリは通常隠しディレクトリなので、うまいこと、やってください
• これをすることで、White spce に関する規約をcommit 直前にgitがチェックしてくれます。こうすることで、自然とすべてのPRがWhite spaceの規約に則ることになります。
  • ちなみに、OpenCVはインデントはスペース4つです。タブは使いません。
• 例えばインデントにタブを使うと、Commit自体がエラーになります

PRを書くコツ†

• 最近、PRを書く際に、テンプレートが表示されるようになりました。
• テンプレートには、大きく、**What does this PR change **という節を書くようになっています
• ここに、分かりやすく
  • このPRで何をしたいのか
  • このPRを何故書いたのか
  • などを分かりやすく説明します
• 私のオススメとしては、この説明文を上手く書けないようなPRは、恐らく細切れなPRに分割した方が良いと思います

BuildBot†

• PRを投げたあとの全体的な流れはこの図が参考になると思います。
• PRを投げたあとは、機械テストが実行されます。
• 2016年5月16日時点では
  • Linux x64ビルド
  • Win7 x64ビルド(VS2013)
  • Win10 x64ビルド(VS2015)
  • Mac
  • Android armeabi-v7a
  • OpenCL
  • Linux x64 Debug
  • Docs
  • iOS
  • の9種類のビルド+テストが実行されます
• オプショナルに
  • OpenCL Intel
  • Win32
  • Arm
  • Android pack
  • などが実行されますが、実行される条件は不明です。
• この機械テストは、自分のローカルで再現可能です。そのことに関してはまたその内書きます
• 全ての自動ビルドに、Warningもなく、エラーもなく、その上で全てのテストがpassすれば、晴れてマージ可能となります。
• ただし、マージされるかどうかは、Reviewが入ります

Review†

• Reviewのためには、Intelのメンバが承認することが必要です。
• マージする前に直すべき点、コードの書き方、テストが抜けている、など様々な指摘が来ますので、真摯に直しましょう。
• ReviewでOKが出ると、Intelのメンバがアサインされます
• アサインされたメンバが絵文字のサムアップ&note{thumb-up:どうもこのサムアップの絵文字はgithub公式のものらしくて、':+1'と書くと自動的に絵文字に変換されるらしい};を書くと、自動的にリポジトリにマージされます
• 当然英語でやり取りをする必要があります。頑張りましょう。

Reviewでの指摘†

• 私の修正は、自分自身でも、イマイチピンと来なかったのですが、指摘で
  • ニュートン法を繰り返すのはオーバースペック
  • 11.054863929748535と11.054862976074219はhex表現で0x4130E0B9と0x4130E0B9で、そもそも1ULPSしか違わない
  • そんなbit exactに計算結果を比較しているのがナンセンス
  • そして、よく見れば等しいかどうかの比較において、doubleとして比較されていたのが原因
  • という指摘がありました。
• 私自身、PRを投げた経験と同じぐらい、この指摘で多くのことを学びました。
• つまり問題の根源は計算でなく、floatの比較にEXPECT_DOUBLE_EQというマクロを使ってることでした。
• なので、そこを適切に直して、私のPRは無事マージされました
• ちなみにEXPECT_FLOAT_EQ内では、hex表現のfloatが、いくつ離れているか(ULPS)という点でtrueかfalseを返します。
• ちなみに、このhex表現のfloatがいくつ離れているか、というのはULPという単位で表されます。
• 丸め誤差を考慮し、最大でULP4つ分まで離れていても同じ浮動小数点である、と判定する仕組みになっています。

マージ(Squash)†

• Reviewや、BuildBotでの修正の過程で複数のcommitがpushされることがあります
• 通常、その過程を全て1つのcommitにまとめる(Squashする)ことが要求されます
  • branch元にgit reset --mixedする
  • pushした先のbranchをmasterにmergeする。その際、--squashオプションを付ける
  • 再度commitする
  • 再度pushする。この際、SHA1値が変わってるので、push --forceする

感想†

• テスト、すげー。そして大事
• 一発でテストに通ると思わないほうが良い。テストでfailしたら恥ずかしいと思うけれど、恥じずに直す
• 何が問題なのか、ちゃんと説明できないなら、良く精査した方が良い

過去に投げたOpenCVのPull Request†

Pull Requests · opencv/opencv

:OpenCV

最新の20件