Closed
Bug 1036873
Opened 7 years ago
Closed 7 years ago
Add support for standard V4L2 FM deemphasis control
Categories
(Core :: Hardware Abstraction Layer (HAL), defect)
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: mwu, Assigned: mwu)
References
Details
Attachments
(1 file, 1 obsolete file)
2.10 KB,
patch
|
Details | Diff | Splinter Review |
V4L2 has support for configuring deemphasis. This patch makes our hal code take advantage of that control. Unfortunately, our kernel headers from bionic are not new enough to define this new control.
Attachment #8453686 -
Flags: review?(dhylands)
Comment 1•7 years ago
|
||
Comment on attachment 8453686 [details] [diff] [review] Add support for standard V4L2 FM deemphasis control Review of attachment 8453686 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Just a couple of minor things I commented on. r=me with those addressed. ::: hal/gonk/GonkFMRadio.cpp @@ +26,5 @@ > #include <stdlib.h> > #include <sys/stat.h> > #include <sys/types.h> > > +#ifndef V4L2_CTRL_CLASS_FM_RX These are the constants that you added due to our headers being too old right? Could you just add a comment that says that (what you said in the bug description is good - I'd just like to see that in the source here. @@ +329,5 @@ > } > > + int emphasis; > + switch (aInfo.preEmphasis()) { > + case 0: nit: case should be indented 2 from the switch according to the coding standars. https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures @@ +339,5 @@ > + case 75: > + emphasis = V4L2_DEEMPHASIS_75_uS; > + break; > + default: > + MOZ_CRASH("Invalid preemphasis setting"); Is this something that can originate in any way from the JS side? If so, a crash seems rather severe. If its purely internal, then the crash is fine.
Attachment #8453686 -
Flags: review?(dhylands) → review+
Assignee | ||
Comment 2•7 years ago
|
||
(In reply to Dave Hylands [:dhylands] from comment #1) > Comment on attachment 8453686 [details] [diff] [review] > Add support for standard V4L2 FM deemphasis control > > Review of attachment 8453686 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good. Just a couple of minor things I commented on. > > r=me with those addressed. > > ::: hal/gonk/GonkFMRadio.cpp > @@ +26,5 @@ > > #include <stdlib.h> > > #include <sys/stat.h> > > #include <sys/types.h> > > > > +#ifndef V4L2_CTRL_CLASS_FM_RX > > These are the constants that you added due to our headers being too old > right? > > Could you just add a comment that says that (what you said in the bug > description is good - I'd just like to see that in the source here. > Will do. > @@ +329,5 @@ > > } > > > > + int emphasis; > > + switch (aInfo.preEmphasis()) { > > + case 0: > > nit: case should be indented 2 from the switch according to the coding > standars. > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/ > Coding_Style#Control_Structures > Oh. This file is already using no indenting for case though. > @@ +339,5 @@ > > + case 75: > > + emphasis = V4L2_DEEMPHASIS_75_uS; > > + break; > > + default: > > + MOZ_CRASH("Invalid preemphasis setting"); > > Is this something that can originate in any way from the JS side? If so, a > crash seems rather severe. > > If its purely internal, then the crash is fine. The JS api doesn't have access to these parameters. Unfortunately, I just realized that nothing on the dom side is setting this properly.. looks like the infrastructure for everything was setup but nobody actually hooked it up. There's also no default value for this AFAICT, so this would actually crash. I'll take out this MOZ_CRASH until that gets resolved.
Comment 3•7 years ago
|
||
(In reply to Michael Wu [:mwu] from comment #2) > (In reply to Dave Hylands [:dhylands] from comment #1) > > @@ +329,5 @@ > > > } > > > > > > + int emphasis; > > > + switch (aInfo.preEmphasis()) { > > > + case 0: > > > > nit: case should be indented 2 from the switch according to the coding > > standars. > > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/ > > Coding_Style#Control_Structures > > > > Oh. This file is already using no indenting for case though. ok - I didn't see that. Consistency is more important
Assignee | ||
Comment 4•7 years ago
|
||
Comment added. MOZ_CRASH is still here because I'm going to fix the issue in bug 1037646 before landing this.
Attachment #8453686 -
Attachment is obsolete: true
Assignee | ||
Comment 5•7 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/f8c2c10aa168
Comment 6•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f8c2c10aa168
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in
before you can comment on or make changes to this bug.
Description
•